Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 118 additions & 0 deletions .github/workflows/qa-review.yml
Original file line number Diff line number Diff line change
@@ -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
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
6 changes: 6 additions & 0 deletions config/qa-review-repos.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"wpeverest": [],
"themegrill": [
"user-registration-pro"
]
}
68 changes: 68 additions & 0 deletions scripts/qa-review-comment.mjs
Original file line number Diff line number Diff line change
@@ -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 = "<!-- tg-autopilot:qa-review -->";
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 <owner/repo> <pr> <body-file>");
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);
});
}
146 changes: 146 additions & 0 deletions scripts/qa-review-gate.mjs
Original file line number Diff line number Diff line change
@@ -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);
});
}
Loading
Loading