diff --git a/.github/workflows/qa-review.yml b/.github/workflows/qa-review.yml new file mode 100644 index 0000000..5766911 --- /dev/null +++ b/.github/workflows/qa-review.yml @@ -0,0 +1,118 @@ +# QA review of a PR, requested by adding tg-autopilot as a reviewer. +# +# Front door: n8n receives GitHub's pull_request `review_requested` webhook, +# pre-filters it, and fires repository_dispatch `qa-review` here (same bridge +# pattern as crisp-investigate-now.yml). workflow_dispatch is the manual +# fallback: GitHub never retries a failed webhook delivery, so if n8n was down +# when a review was requested, re-run it by hand. +# +# The dispatch payload is UNTRUSTED (anyone who finds the n8n URL, or holds a +# dispatch token, can send anything). The gate job re-verifies everything +# against the live GitHub API; nothing after it reads the payload again. +# Untrusted values only ever reach scripts through `env:`, never through +# `${{ }}` interpolation into a shell line. +# +# Design: QA-REVIEW.md (to be added). Pilot repo: themegrill/user-registration-pro, +# see config/qa-review-repos.json. +name: QA review + +on: + repository_dispatch: + types: [qa-review] + workflow_dispatch: + inputs: + repo: + description: "owner/name, must be listed in config/qa-review-repos.json" + required: true + type: string + pr: + description: "Pull request number" + required: true + type: string + head_sha: + description: "Optional: refuse to run unless the PR head is exactly this commit" + required: false + type: string + default: "" + post_comment: + description: "Post/update the sticky PR comment (off by default so manual tests stay silent)" + required: false + type: boolean + default: false + +# Never cancel: a denied or duplicate request must not kill a good in-flight +# review. GitHub keeps at most one pending run per group (the newest). +concurrency: + group: qa-review-${{ github.event.client_payload.repo || inputs.repo }}-${{ github.event.client_payload.pr || inputs.pr }} + cancel-in-progress: false + +permissions: + contents: read + +jobs: + gate: + name: Gate + runs-on: ubuntu-latest + outputs: + allowed: ${{ steps.gate.outputs.allowed }} + head_sha: ${{ steps.gate.outputs.head_sha }} + env: + QA_REPO: ${{ github.event.client_payload.repo || inputs.repo }} + QA_PR: ${{ github.event.client_payload.pr || inputs.pr }} + QA_HEAD_SHA: ${{ github.event.client_payload.head_sha || inputs.head_sha }} + # For a manual run the requester is whoever clicked "Run workflow". + QA_REQUESTER: ${{ github.event.client_payload.requester || github.actor }} + POST_COMMENT: ${{ github.event_name == 'repository_dispatch' || inputs.post_comment }} + # Each PAT is scoped to one org; the scripts pick by the repo's owner. + BOT_TOKEN: ${{ secrets.BOT_TOKEN }} + BOT_TOKEN_THEMEGRILL: ${{ secrets.BOT_TOKEN_THEMEGRILL }} + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version: "20" + + - name: Gate (allowlist, PR state, same-repo, head sha, requester permission) + id: gate + run: node scripts/qa-review-gate.mjs + + - name: Summarize gate result + if: always() + env: + ALLOWED: ${{ steps.gate.outputs.allowed }} + CODE: ${{ steps.gate.outputs.code }} + REASON: ${{ steps.gate.outputs.reason }} + run: | + { + echo "### QA review gate" + echo + echo "- Repo: \`$QA_REPO\` PR: \`$QA_PR\` requester: \`$QA_REQUESTER\`" + echo "- Allowed: \`${ALLOWED:-error}\` (\`${CODE:-n/a}\`)" + echo "- Reason: ${REASON:-gate step failed before deciding; see its log}" + } >> "$GITHUB_STEP_SUMMARY" + + # Only a permission denial is worth a PR comment (the requester would + # otherwise see nothing). Other denials (draft, stale, fork...) just log. + - name: Tell the requester they lack access + if: steps.gate.outputs.allowed == 'false' && steps.gate.outputs.notify == 'true' && env.POST_COMMENT == 'true' + env: + REASON: ${{ steps.gate.outputs.reason }} + run: | + printf '%s\n' "QA review not started: $REASON" > body.md + node scripts/qa-review-comment.mjs "$QA_REPO" "$QA_PR" body.md + + # Placeholder until the static + LLM jobs exist (next step). Deliberately + # says so: a pilot PR must not look like it got a real review. + - name: Acknowledge the request + if: steps.gate.outputs.allowed == 'true' && env.POST_COMMENT == 'true' + env: + HEAD_SHA: ${{ steps.gate.outputs.head_sha }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + printf '%s\n' \ + "**QA review requested** by @$QA_REQUESTER for \`${HEAD_SHA:0:7}\`." \ + "" \ + "Request accepted. The review pipeline is not wired up yet, so no findings will follow. Run: $RUN_URL" \ + > body.md + node scripts/qa-review-comment.mjs "$QA_REPO" "$QA_PR" body.md diff --git a/CHANGELOG.md b/CHANGELOG.md index f6559c5..e8c49a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ 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: 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. + ## 2026-10-05 — wp.org triage files issues in the pro repo first Issues now go to the product's `-pro` repo when it has one, otherwise to the free repo (`issue_repos` in `wporg/config/products.json`, replacing `fallback_repos`). The agent still reads the free repo's code and searches both repos for existing issues (Crisp files free-edition issues in the free repos, so a match can live there). The old "probe the free repo, fall back to pro" behavior is gone: the job now only checks that the token can write to the chosen issue repo and fails loudly if not (the topic is already marked seen, so re-run it by hand). diff --git a/config/qa-review-repos.json b/config/qa-review-repos.json new file mode 100644 index 0000000..9c39917 --- /dev/null +++ b/config/qa-review-repos.json @@ -0,0 +1,6 @@ +{ + "wpeverest": [], + "themegrill": [ + "user-registration-pro" + ] +} diff --git a/scripts/qa-review-comment.mjs b/scripts/qa-review-comment.mjs new file mode 100644 index 0000000..c72af5c --- /dev/null +++ b/scripts/qa-review-comment.mjs @@ -0,0 +1,68 @@ +// Creates or updates THE single QA-review comment on a PR (never one per run). +// +// Identity rule (write-safe-bot-workflow skill): the machine user's API type is +// "User", not "Bot", so our own comment is recognised by login + a stable body +// marker, never by user.type. +// +// Loop safety: nothing here can re-trigger anything. The only trigger for QA +// review is a review_requested event, never a comment, so the text below is +// free to say whatever it needs to. +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { ghTokenForRepo } from "./github-client.mjs"; +import { BOT_LOGIN } from "./qa-review-gate.mjs"; + +export const MARKER = ""; +const MAX_BODY = 60000; // GitHub's hard limit is 65536 chars + +export function withMarker(body) { + const trimmed = body.length > MAX_BODY ? body.slice(0, MAX_BODY) + "\n\n_(truncated)_" : body; + return `${MARKER}\n${trimmed}`; +} + +export function findSticky(comments) { + // Newest-first would be wrong if an old duplicate exists; take the first + // (oldest) so repeated runs converge on one comment. + return comments.find((c) => c.user?.login === BOT_LOGIN && c.body?.startsWith(MARKER)); +} + +export async function upsertSticky({ repo, pr, body, token, fetchImpl = fetch }) { + const headers = { + Authorization: `Bearer ${token}`, + Accept: "application/vnd.github+json", + "Content-Type": "application/json", + }; + const base = `https://api.github.com/repos/${repo}/issues`; + + let existing; + for (let page = 1; page <= 10 && !existing; page++) { + const res = await fetchImpl(`${base}/${pr}/comments?per_page=100&page=${page}`, { headers }); + if (!res.ok) throw new Error(`List comments failed on ${repo}#${pr}: ${res.status} ${await res.text()}`); + const batch = await res.json(); + existing = findSticky(batch); + if (batch.length < 100) break; + } + + const payload = JSON.stringify({ body: withMarker(body) }); + const res = existing + ? await fetchImpl(`${base}/comments/${existing.id}`, { method: "PATCH", headers, body: payload }) + : await fetchImpl(`${base}/${pr}/comments`, { method: "POST", headers, body: payload }); + if (!res.ok) throw new Error(`Upsert comment failed on ${repo}#${pr}: ${res.status} ${await res.text()}`); + return { updated: Boolean(existing), url: (await res.json()).html_url }; +} + +async function main() { + const [repo, pr, bodyFile] = process.argv.slice(2); + if (!repo || !pr || !bodyFile) throw new Error("usage: qa-review-comment.mjs "); + const token = ghTokenForRepo(repo); + if (!token) throw new Error(`No bot token configured for ${repo}'s org`); + const out = await upsertSticky({ repo, pr, body: readFileSync(bodyFile, "utf8"), token }); + console.log(`${out.updated ? "Updated" : "Created"} QA review comment: ${out.url}`); +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + main().catch((err) => { + console.error(`::error::${err.message}`); + process.exit(1); + }); +} diff --git a/scripts/qa-review-gate.mjs b/scripts/qa-review-gate.mjs new file mode 100644 index 0000000..913eed1 --- /dev/null +++ b/scripts/qa-review-gate.mjs @@ -0,0 +1,146 @@ +// QA review gate: the AUTHORITATIVE check that a review request is allowed to +// run. n8n pre-filters too, but its payload is untrusted (anyone who finds the +// webhook URL can POST to it, and manual workflow_dispatch skips n8n entirely), +// so everything is re-verified here against the live GitHub API. +// +// Two outcomes, deliberately different: +// - DENIED (allowed=false): a legitimate "no" (draft, stale SHA, no write +// access...). Exit 0 so later steps can report it; the job stays green. +// - ERROR (thrown): the token can't read the repo/PR, wrong identity, etc. +// Exit 1. A silently-green job that did nothing is exactly the failure mode +// the 2026-10-05 wp.org CHANGELOG entry describes. +import { appendFileSync, readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { ghTokenForRepo } from "./github-client.mjs"; + +export const BOT_LOGIN = "tg-autopilot"; + +// Same ranking as copilot-review-on-comment.yml. Each review spends LLM money, +// so triage-level users (who GitHub lets request reviewers) are NOT enough. +const RANK = { admin: 4, maintain: 3, write: 2, triage: 1 }; +export const MIN_PERMISSION = "write"; + +// repo/pr arrive from a dispatch payload: validate strictly before they go +// anywhere near a URL, a filename or a log line. +const REPO_RE = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; +const PR_RE = /^[1-9][0-9]{0,8}$/; +const SHA_RE = /^[0-9a-f]{40}$/; +const USER_RE = /^[A-Za-z0-9](?:[A-Za-z0-9-]{0,38})$/; + +export function validateInput({ repo, pr, requester, expectedSha }) { + if (!REPO_RE.test(repo ?? "") || repo.split("/").some((p) => p === "." || p === "..")) { + return "repo must look like owner/name"; // "../x" matches the regex but would traverse in API URLs + } + if (!PR_RE.test(String(pr ?? ""))) return "pr must be a positive integer"; + if (!USER_RE.test(requester ?? "")) return "requester is not a valid GitHub login"; + if (expectedSha && !SHA_RE.test(expectedSha)) return "head_sha must be a 40-char hex sha"; + return null; +} + +export function isAllowlisted(config, repo) { + const [owner, name] = repo.split("/"); + const list = config?.[owner]; + return Array.isArray(list) && list.includes(name); +} + +export function hasMinPermission(permission, min = MIN_PERMISSION) { + return (RANK[permission] ?? 0) >= RANK[min]; +} + +// Pure decision over already-fetched facts, so every rule is unit-testable. +export function decide({ pr, permission, requester, expectedSha }) { + if (requester.toLowerCase() === BOT_LOGIN) { + return deny("self", "Request was made by the bot itself; ignoring to avoid a loop."); + } + if (pr.state !== "open") return deny("not-open", `PR is ${pr.state}.`); + if (pr.draft) return deny("draft", "PR is a draft."); + if (!pr.head?.repo || pr.head.repo.full_name.toLowerCase() !== pr.base.repo.full_name.toLowerCase()) { + // Fork (or deleted fork): later phases run PR-influenced tooling, so only + // same-repo branches are in scope until a maintainer-approval flow exists. + return deny("fork", "PRs from forks are not supported yet."); + } + if (expectedSha && expectedSha !== pr.head.sha) { + return deny("stale", `PR head moved (requested ${expectedSha.slice(0, 7)}, now ${pr.head.sha.slice(0, 7)}). Re-request the review.`); + } + if (!hasMinPermission(permission)) { + return deny("permission", `@${requester} needs write access (or higher) to request a QA review.`, true); + } + return { allowed: true, code: "ok", reason: "ok", headSha: pr.head.sha, baseRef: pr.base.ref }; +} + +function deny(code, reason, notify = false) { + return { allowed: false, code, reason, notify }; +} + +export async function runGate({ repo, pr, requester, expectedSha, config, api }) { + const bad = validateInput({ repo, pr, requester, expectedSha }); + if (bad) return deny("bad-input", bad); + if (!isAllowlisted(config, repo)) return deny("not-allowlisted", `${repo} is not enrolled in QA review.`); + + const bot = await api.whoami(); + if (bot.toLowerCase() !== BOT_LOGIN) { + throw new Error(`Token for ${repo} authenticates as "${bot}", expected "${BOT_LOGIN}". Wrong secret for this org?`); + } + const prData = await api.getPr(repo, pr); + const permission = await api.getPermission(repo, requester); + return decide({ pr: prData, permission, requester, expectedSha }); +} + +export function makeApi(token, fetchImpl = fetch) { + const headers = { Authorization: `Bearer ${token}`, Accept: "application/vnd.github+json" }; + const get = async (path) => { + const res = await fetchImpl(`https://api.github.com${path}`, { headers }); + if (!res.ok) throw new Error(`GitHub GET ${path} failed: ${res.status} ${await res.text()}`); + return res.json(); + }; + return { + whoami: async () => (await get("/user")).login, + getPr: (repo, n) => get(`/repos/${repo}/pulls/${n}`), + // Not-a-collaborator comes back as 404/200-with-"none" depending on the + // account; both must read as "no access", not as an infrastructure error. + getPermission: async (repo, user) => { + const res = await fetchImpl(`https://api.github.com/repos/${repo}/collaborators/${user}/permission`, { headers }); + if (res.status === 404) return "none"; + if (!res.ok) throw new Error(`GitHub permission lookup failed: ${res.status} ${await res.text()}`); + return (await res.json()).permission; + }, + }; +} + +async function main() { + const { QA_REPO, QA_PR, QA_REQUESTER, QA_HEAD_SHA, GITHUB_OUTPUT } = process.env; + const config = JSON.parse(readFileSync(new URL("../config/qa-review-repos.json", import.meta.url), "utf8")); + // Validate before choosing a token: ghTokenForRepo() needs a sane repo string. + const early = validateInput({ repo: QA_REPO, pr: QA_PR, requester: QA_REQUESTER, expectedSha: QA_HEAD_SHA }); + const token = early ? undefined : ghTokenForRepo(QA_REPO); + if (!early && !token) throw new Error(`No bot token configured for ${QA_REPO}'s org`); + + const result = await runGate({ + repo: QA_REPO, + pr: QA_PR, + requester: QA_REQUESTER, + expectedSha: QA_HEAD_SHA || undefined, + config, + api: token ? makeApi(token) : null, + }); + + console.log(JSON.stringify(result)); + if (GITHUB_OUTPUT) { + const lines = [ + `allowed=${result.allowed}`, + `code=${result.code}`, + // Reasons are built from validated fields only, but keep outputs single-line regardless. + `reason=${String(result.reason).replace(/[\r\n]+/g, " ")}`, + `notify=${Boolean(result.notify)}`, + `head_sha=${result.headSha ?? ""}`, + ]; + appendFileSync(GITHUB_OUTPUT, lines.join("\n") + "\n"); + } +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + main().catch((err) => { + console.error(`::error::${err.message}`); + process.exit(1); + }); +} diff --git a/scripts/qa-review-gate.test.mjs b/scripts/qa-review-gate.test.mjs new file mode 100644 index 0000000..e608f45 --- /dev/null +++ b/scripts/qa-review-gate.test.mjs @@ -0,0 +1,109 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { validateInput, isAllowlisted, hasMinPermission, decide, runGate, BOT_LOGIN } from "./qa-review-gate.mjs"; +import { findSticky, withMarker, upsertSticky, MARKER } from "./qa-review-comment.mjs"; + +const SHA = "a".repeat(40); +const OTHER = "b".repeat(40); +const CONFIG = { themegrill: ["user-registration-pro"], wpeverest: [] }; +const basePr = (over = {}) => ({ + state: "open", + draft: false, + head: { sha: SHA, repo: { full_name: "themegrill/user-registration-pro" } }, + base: { ref: "develop", repo: { full_name: "themegrill/user-registration-pro" } }, + ...over, +}); +const api = ({ pr = basePr(), permission = "write", login = BOT_LOGIN } = {}) => ({ + whoami: async () => login, + getPr: async () => pr, + getPermission: async () => permission, +}); +const gate = (over = {}, apiOver = {}) => + runGate({ repo: "themegrill/user-registration-pro", pr: "12", requester: "alice", config: CONFIG, api: api(apiOver), ...over }); + +test("validateInput rejects injection-shaped values", () => { + const ok = { repo: "o/r", pr: "1", requester: "alice" }; + assert.equal(validateInput(ok), null); + assert.ok(validateInput({ ...ok, repo: "o/r; rm -rf /" })); + assert.ok(validateInput({ ...ok, repo: "../../x" })); + assert.ok(validateInput({ ...ok, repo: "../x" })); // regex-legal but path traversal in API URLs + assert.ok(validateInput({ ...ok, repo: "o/.." })); + assert.ok(validateInput({ ...ok, repo: "o/r/extra" })); + assert.ok(validateInput({ ...ok, pr: "0" })); + assert.ok(validateInput({ ...ok, pr: "1 2" })); + assert.ok(validateInput({ ...ok, requester: "a b" })); + assert.ok(validateInput({ ...ok, requester: "$(id)" })); + assert.ok(validateInput({ ...ok, expectedSha: "abc" })); +}); + +test("allowlist is per-org and exact", () => { + assert.equal(isAllowlisted(CONFIG, "themegrill/user-registration-pro"), true); + assert.equal(isAllowlisted(CONFIG, "themegrill/user-registration"), false); + assert.equal(isAllowlisted(CONFIG, "wpeverest/user-registration-pro"), false); + assert.equal(isAllowlisted(CONFIG, "other/user-registration-pro"), false); +}); + +test("permission ranking: triage and read are not enough", () => { + for (const p of ["admin", "maintain", "write"]) assert.equal(hasMinPermission(p), true, p); + for (const p of ["triage", "read", "none", undefined]) assert.equal(hasMinPermission(p), false, String(p)); +}); + +test("happy path returns head sha and base ref", async () => { + const r = await gate({ expectedSha: SHA }); + assert.equal(r.allowed, true); + assert.equal(r.headSha, SHA); + assert.equal(r.baseRef, "develop"); +}); + +test("denials: code and whether the requester is notified", async () => { + assert.deepEqual(pick(await gate({}, { permission: "triage" })), { code: "permission", notify: true }); + assert.deepEqual(pick(await gate({}, { pr: basePr({ draft: true }) })), { code: "draft", notify: false }); + assert.deepEqual(pick(await gate({}, { pr: basePr({ state: "closed" }) })), { code: "not-open", notify: false }); + assert.deepEqual(pick(await gate({ expectedSha: OTHER })), { code: "stale", notify: false }); + assert.deepEqual(pick(await gate({ requester: BOT_LOGIN })), { code: "self", notify: false }); + assert.deepEqual(pick(await gate({ repo: "themegrill/other" })), { code: "not-allowlisted", notify: false }); + assert.deepEqual(pick(await gate({ pr: "x" })), { code: "bad-input", notify: false }); +}); + +test("fork PRs (and deleted forks) are denied", async () => { + const fork = basePr({ head: { sha: SHA, repo: { full_name: "mallory/user-registration-pro" } } }); + assert.equal((await gate({}, { pr: fork })).code, "fork"); + const deleted = basePr({ head: { sha: SHA, repo: null } }); + assert.equal((await gate({}, { pr: deleted })).code, "fork"); +}); + +test("wrong token identity is an ERROR, not a denial", async () => { + await assert.rejects(gate({}, { login: "lihsaa591" }), /expected "tg-autopilot"/); +}); + +test("decide never trusts the bot's own request even with write access", () => { + assert.equal(decide({ pr: basePr(), permission: "admin", requester: "TG-Autopilot" }).code, "self"); +}); + +test("sticky comment: matched by login AND marker, never by marker alone", () => { + const mine = { id: 1, user: { login: BOT_LOGIN }, body: `${MARKER}\nhi` }; + const spoof = { id: 2, user: { login: "mallory" }, body: `${MARKER}\nfake` }; + const unrelated = { id: 3, user: { login: BOT_LOGIN }, body: "Build ZIP ready" }; + assert.equal(findSticky([spoof, unrelated, mine]).id, 1); + assert.equal(findSticky([spoof, unrelated]), undefined); + assert.ok(withMarker("x").startsWith(MARKER)); +}); + +test("upsertSticky: creates when absent, PATCHes the existing one when present", async () => { + const calls = []; + const mk = (existing) => async (url, init = {}) => { + calls.push({ url, method: init.method ?? "GET" }); + const json = init.method ? { html_url: "u" } : existing ? [{ id: 9, user: { login: BOT_LOGIN }, body: MARKER }] : []; + return { ok: true, status: 200, json: async () => json, text: async () => "" }; + }; + const created = await upsertSticky({ repo: "o/r", pr: 1, body: "b", token: "t", fetchImpl: mk(false) }); + assert.equal(created.updated, false); + assert.equal(calls.at(-1).method, "POST"); + const updated = await upsertSticky({ repo: "o/r", pr: 1, body: "b", token: "t", fetchImpl: mk(true) }); + assert.equal(updated.updated, true); + assert.deepEqual(calls.at(-1), { url: "https://api.github.com/repos/o/r/issues/comments/9", method: "PATCH" }); +}); + +function pick(r) { + return { code: r.code, notify: r.notify }; +}