From f1f6225780faa41683575b898666e7f042b114c5 Mon Sep 17 00:00:00 2001 From: Relayflow Lead Date: Thu, 17 Sep 2026 15:01:43 -0700 Subject: [PATCH 1/3] examples(pr-reviewer): port the wepost PR reviewer to flows v2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wepost-no/agents `review/agent.ts` reviewer as a v2 relayflow. Same review prompt and gates; every step the v4 platform performed on the agent's behalf is now a journaled step the agent cannot forge: PR state via REST in `f.run`, checkout and diff via git steps, one `f.agent` gated on the review file, the repository's tests run OUTSIDE the agent, a push only when that run is green (else discard + advisory), READY only when the agent said so AND the tests were green AND GitHub's live state agrees, and the comment/merge as journaled `f.github` effects. The pure decision functions are ported verbatim and exported from the flow file (one file: Cloud receives a single source and resolves no sibling imports); the upstream test suite comes along (38) plus six for the v2 seams. Harness-retry/exit-code diagnostics are not ported — the kernel owns retries and leases. Proven end to end through the real kernel and authored runtime with two stand-ins (a curl shim answering GitHub API fixtures, a wrapper agent writing review.md): happy path pushes the review commit to the PR branch and posts the comment with the sentinel stripped; red tests leave the branch untouched, discard the edits and post an advisory with no ready line; a draft PR stops after one API read. The proof caught three bugs before merge: `checkout FETCH_HEAD` after a two-ref fetch took the base branch, `git add -A ':!.workforce'` exits 1 on a gitignored directory, and the READY sentinel leaked into the body without trimEnd(). Gated with `subprocess_gate` until `artifact_exists` lands (one-line swap, marked TODO). Cloud PR-event admission, the PR-head grant, and the `check_run`/`issue_comment` trigger vocabulary are documented as not yet wired. Co-Authored-By: Claude Opus 5 (1M context) --- examples/README.md | 1 + examples/pr-reviewer/README.md | 138 +++ .../pr-reviewer/evidence/agent-wrapper.mjs | 12 + .../pr-reviewer/evidence/api-calls-draft.txt | 1 + .../pr-reviewer/evidence/api-calls-red.txt | 11 + .../evidence/api-calls-success.txt | 13 + examples/pr-reviewer/evidence/curl-shim.sh | 14 + .../evidence/origin-feature-after.txt | 3 + examples/pr-reviewer/flows.json | 1 + examples/pr-reviewer/package.json | 9 + examples/pr-reviewer/pr-reviewer.flow.ts | 884 ++++++++++++++++++ examples/pr-reviewer/tests/pr-state.test.ts | 609 ++++++++++++ examples/tsconfig.json | 3 +- 13 files changed, 1698 insertions(+), 1 deletion(-) create mode 100644 examples/pr-reviewer/README.md create mode 100755 examples/pr-reviewer/evidence/agent-wrapper.mjs create mode 100644 examples/pr-reviewer/evidence/api-calls-draft.txt create mode 100644 examples/pr-reviewer/evidence/api-calls-red.txt create mode 100644 examples/pr-reviewer/evidence/api-calls-success.txt create mode 100755 examples/pr-reviewer/evidence/curl-shim.sh create mode 100644 examples/pr-reviewer/evidence/origin-feature-after.txt create mode 100644 examples/pr-reviewer/flows.json create mode 100644 examples/pr-reviewer/package.json create mode 100644 examples/pr-reviewer/pr-reviewer.flow.ts create mode 100644 examples/pr-reviewer/tests/pr-state.test.ts diff --git a/examples/README.md b/examples/README.md index 606dad24d..9a1c8cb9a 100644 --- a/examples/README.md +++ b/examples/README.md @@ -10,6 +10,7 @@ For a working local starting point, use the [small agent starter](../README.md) |---|---|---|---:| | [dependency-upgrade-bot](dependency-upgrade-bot/) | **BLOCKED** | SDK refuses unsupported `budget` header before entering the body; exit 2 | [5.138s](../docs/evidence/ws13/review/gallery/gallery-dependency-upgrade-bot.txt) | | [pr-review-pipeline](pr-review-pipeline/) | **BLOCKED** | SDK refuses unsupported `budget` header before entering the body; exit 2 | [3.539s](../docs/evidence/ws13/review/gallery/gallery-pr-review-pipeline.txt) | +| [pr-reviewer](pr-reviewer/) | **PASS (local, stand-ins)** | The wepost PR reviewer as a v2 flow: 17 journaled steps end to end through the real kernel with a wrapper agent and an API shim; happy, red-tests and draft paths proven. See its README. | — | | [research](research/) | **PASS** | All model probes passed; three lane reports and synthesis produced; exit 0, `completionReason: synthesized` | [690.935s](../docs/evidence/ws13/followup/default-budget/gallery-research.txt) | **Correction:** the previously listed 0.138s and 0.143s captures used a stale diff --git a/examples/pr-reviewer/README.md b/examples/pr-reviewer/README.md new file mode 100644 index 000000000..7ae6d1740 --- /dev/null +++ b/examples/pr-reviewer/README.md @@ -0,0 +1,138 @@ +# pr-reviewer + +The [wepost-no/agents `review/agent.ts`](https://github.com/wepost-no/agents/blob/main/review/agent.ts) +PR reviewer, ported to a v2 relayflow. Same review prompt, same gates, same +"post the review; say it's a human's turn only when it really is" contract — +with every step the v4 platform did on the agent's behalf turned into a +journaled step the agent cannot forge. + +**Like I'm 5:** a robot reads your pull request and writes a review. It's only +allowed to fix boring things (typos, formatting). Then a *different* part of +the machine runs your tests — the robot doesn't get to say whether they +passed. If the tests are green, the boring fixes are pushed; if they're red, +the fixes are thrown away and the review says so. The review is posted, and +it only says "ready for a human" when the robot said so **and** the tests +were green **and** GitHub agrees. + +## The shape + +``` +PR state (REST) → skip gates → checkout PR head + write .workforce/{pr.diff,context.json,threads.json} + → f.agent("review") [gated: review.md exists] + → f.run(npm test) OUTSIDE the agent + → green: commit + push mechanical fixes to the PR branch | red: discard + advisory + → READY? = agent said so ∧ tests green ∧ live PR state allows it + → f.github.comment (journaled effect) → done +``` + +An approval event from an allowlisted approver takes the short path: +`f.github.mergePullRequest` and done. + +| v4 (`defineAgent`) | here | +| --- | --- | +| relayfile VFS `pulls/N/meta.json`, `checks/_summary.json`, `reviews/*.json` | `f.run` + `curl` against the REST API, parsed in TS (`prReviewStateFromRest`) | +| cloud materialized the checkout and `.workforce/pr.diff` | `f.run` git steps | +| `ctx.harness.run({ prompt })` | one `f.agent("review", …)` step, gated on `.workforce/review.md` | +| prompt asks the agent to run the tests | `f.run(npm test, { timeout: "15m" })` — the exit code is the kernel's | +| cloud commits/pushes whatever the agent left | the flow pushes **only** after its own test run is green; otherwise discards and posts an advisory | +| `runReviewHarnessWithRetry` on exit 137/143, log plumbing | not ported: the kernel owns retries and leases; the journal is the log | +| `githubClient().comment / mergePullRequest` | `f.github.comment / mergePullRequest` (journaled, exactly-once) | + +The pure decision functions (`evaluateMergeOnGreenState`, +`prReadyStateAllowsHumanReview`, `reviewAuthorAllowlistDecision`, +`readPr`, the prompts, …) are ported verbatim and exported from the flow +file; [tests/pr-state.test.ts](tests/pr-state.test.ts) carries the upstream +suite (38 tests) plus six for the v2 seams (44 total, `npm test`). + +One file on purpose: Cloud receives a single authored source and does not +resolve sibling imports, so the helpers live below the flow. + +## Input + +```json +{ + "owner": "AgentWorkforce", "repo": "flows", "number": 440, + "approvers": "khaliqgant", + "reviewAuthors": "", "skipLabels": "no-agent-relay-review", + "reviewerCli": "claude", "githubTransport": "helper" +} +``` + +- `approvers` — logins whose approval event merges. Empty: any approval. +- `reviewAuthors` — only review PRs by these logins. Empty: everyone. +- `reviewerCli` — `claude` (default), `codex`, or a custom wrapper path. +- `githubTransport` — `helper` (default) uses the journaled `f.github` effect, + which needs a relayfile GitHub mount (Cloud has one). `curl` posts through + the REST API with `$GH_TOKEN` from a deterministic step, for a local + checkout without a mount. +- `event` — a GitHub webhook payload, when one launched the run. + +## Run it locally + +From a checkout of the repository the PR belongs to: + +```sh +export GH_TOKEN=$(gh auth token) # in the SAME shell that starts the daemon, see below +flows check examples/pr-reviewer/pr-reviewer.flow.ts +flows run examples/pr-reviewer/pr-reviewer.flow.ts --local-agent \ + --input '{"owner":"o","repo":"r","number":7,"approvers":"you","githubTransport":"curl"}' +``` + +Deterministic steps run with the **daemon's** environment, not the CLI's: +`relayflowd` is spawned by the first `flows run` in a data directory and +outlives it. Set `GH_TOKEN` before that first run, or restart the daemon. + +## Run it on Cloud + +```sh +cd && flows run --cloud --sync-code --wait \ + examples/pr-reviewer/pr-reviewer.flow.ts \ + --input '{"owner":"o","repo":"r","number":7,"approvers":"you"}' +``` + +The synced tree is the checkout; the sandbox has a relayfile GitHub mount, so +the default `helper` transport applies. `GH_TOKEN` for the REST reads comes +from the sandbox's repository credential. + +## What is proven, and how + +Everything below ran through the real kernel and the real authored runtime +(`flows run … --local-agent`, v2.0.16), with two stand-ins so nothing touched +a real repository: a `curl` on `PATH` that answers GitHub API shapes from +fixtures and logs every call ([evidence/curl-shim.sh](evidence/curl-shim.sh)), +and a wrapper agent CLI that writes `review.md` and one "mechanical" edit +([evidence/agent-wrapper.mjs](evidence/agent-wrapper.mjs)). The PR lived in a +bare scratch `origin` with `refs/pull/7/head`. + +| Path | Result | +| --- | --- | +| happy: tests green | 17 steps `success`; review commit pushed to the PR branch ([origin-feature-after.txt](evidence/origin-feature-after.txt)); comment posted with the READY sentinel stripped and the `:white_check_mark:` line present ([api-calls-success.txt](evidence/api-calls-success.txt)) | +| red: `npm test` exits 1 | 14 steps `success`; PR branch untouched; edits discarded; advisory section posted; **no** ready line ([api-calls-red.txt](evidence/api-calls-red.txt)) | +| draft PR | 3 steps `success`; one API read, no checkout, no agent, no comment ([api-calls-draft.txt](evidence/api-calls-draft.txt)) | + +Three bugs the proof caught before anyone else could: `checkout FETCH_HEAD` +after a two-ref fetch silently took the base branch (now a named +`refs/remotes/origin/pr-N`); `git add -A ':!.workforce'` exits 1 when the +directory is gitignored (now `add -A` + `reset -- .workforce`); and the READY +sentinel survived into the posted body without a `trimEnd()`. + +Not proven here, honestly: a real Claude session writing a real review (the +wrapper stands in for it), the `helper` transport (needs a relayfile GitHub +mount), and the merge path (needs an approval event). + +## Not yet wired + +- **Cloud trigger path.** `flows deploy` listeners launch on + `issues.opened`/`issues.labeled` only; PR events are filtered out before + launch, and the repository grant checks out the default branch. Until + Cloud admits PR events and grants the PR head, run this on Cloud with + `--sync-code` from a checkout, or locally. +- **`check_run.completed` / `issue_comment.created`.** Merge-on-green and the + `@relay fix conflicts` directive are ported as functions + (`evaluateMergeOnGreenState`, `matchesConflictDirective`, + `isAuthorizedConflictCommander`) and tested, but the generated trigger + vocabulary has no such events yet. +- **`artifact_exists`.** The review step is gated with + `{ type: "subprocess_gate", command: "test -s .workforce/review.md" }`. + When the `artifact_exists` named gate lands, the swap is one line: + `.gate({ type: "artifact_exists", path: REVIEW_FILE })`. diff --git a/examples/pr-reviewer/evidence/agent-wrapper.mjs b/examples/pr-reviewer/evidence/agent-wrapper.mjs new file mode 100755 index 000000000..5b2ebed85 --- /dev/null +++ b/examples/pr-reviewer/evidence/agent-wrapper.mjs @@ -0,0 +1,12 @@ +#!/usr/bin/env node +import { receiveWrapperRequest } from '/testdata/preflight/wrapper-session.mjs'; +import { writeFileSync, mkdirSync, existsSync } from 'node:fs'; +if (process.argv[2] === 'auth') process.exit(0); +const request = await receiveWrapperRequest(); +if (request) { + if (!existsSync('.workforce/pr.diff')) { console.error('no diff materialized'); process.exit(3); } + mkdirSync('.workforce', { recursive: true }); + writeFileSync('.workforce/review.md', '## Review\nLooks fine.\n\n## Addressed comments\n- none\nREADY\n'); + writeFileSync('a.js', 'export const a = 2; // reviewed\n'); // a "mechanical" edit + console.log('review-written'); process.exit(0); +} diff --git a/examples/pr-reviewer/evidence/api-calls-draft.txt b/examples/pr-reviewer/evidence/api-calls-draft.txt new file mode 100644 index 000000000..257488f2f --- /dev/null +++ b/examples/pr-reviewer/evidence/api-calls-draft.txt @@ -0,0 +1 @@ +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7 diff --git a/examples/pr-reviewer/evidence/api-calls-red.txt b/examples/pr-reviewer/evidence/api-calls-red.txt new file mode 100644 index 000000000..b93130624 --- /dev/null +++ b/examples/pr-reviewer/evidence/api-calls-red.txt @@ -0,0 +1,11 @@ +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/8 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/8/comments?per_page=100 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/8/reviews?per_page=100 +-sf -X POST -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/issues/8/comments -d {"body":"## Review +Looks fine. + +## Addressed comments +- none + +## Advisory +The reviewer's edits were discarded: the repository's test command was red in the review sandbox (see .workforce/test.log). The proposed changes are described above; nothing was pushed."} diff --git a/examples/pr-reviewer/evidence/api-calls-success.txt b/examples/pr-reviewer/evidence/api-calls-success.txt new file mode 100644 index 000000000..2605012e4 --- /dev/null +++ b/examples/pr-reviewer/evidence/api-calls-success.txt @@ -0,0 +1,13 @@ +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/comments?per_page=100 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/reviews?per_page=100 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/commits/6dee8f6fa764906cf2c7039ff217a991a6eb315b/check-runs?per_page=100 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/reviews?per_page=100 +-sf -X POST -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/issues/7/comments -d {"body":"## Review +Looks fine. + +## Addressed comments +- none + +:white_check_mark: This PR is ready for your review."} diff --git a/examples/pr-reviewer/evidence/curl-shim.sh b/examples/pr-reviewer/evidence/curl-shim.sh new file mode 100755 index 000000000..d3d6e5c57 --- /dev/null +++ b/examples/pr-reviewer/evidence/curl-shim.sh @@ -0,0 +1,14 @@ +#!/bin/sh +# Stands in for api.github.com. Fixtures come from env: PRPROOF_HEAD (sha), +# PRPROOF_REF (head branch), PRPROOF_DRAFT (true/false). Every call is logged. +url=""; for a in "$@"; do case "$a" in https://api.github.com/*) url="$a";; esac; done +echo "$*" >> "$PRPROOF_LOG" +draft="${PRPROOF_DRAFT:-false}" +case "$url" in + */pulls/[0-9]*/comments*) printf '[]' ;; + */pulls/[0-9]*/reviews*) printf '[]' ;; + */pulls/[0-9]*) printf '{"state":"open","draft":%s,"merged":false,"mergeable":true,"mergeable_state":"clean","user":{"login":"octocat"},"labels":[],"head":{"sha":"%s","ref":"%s"},"base":{"ref":"main"},"html_url":"https://github.com/o/r/pull/7","title":"feature"}' "$draft" "$PRPROOF_HEAD" "$PRPROOF_REF" ;; + */commits/*/check-runs*) printf '{"check_runs":[{"name":"ci","status":"completed","conclusion":"success"}]}' ;; + */issues/[0-9]*/comments) printf '{"id":1}' ;; + *) echo "unexpected $url" >&2; exit 22 ;; +esac diff --git a/examples/pr-reviewer/evidence/origin-feature-after.txt b/examples/pr-reviewer/evidence/origin-feature-after.txt new file mode 100644 index 000000000..4ee4cabbe --- /dev/null +++ b/examples/pr-reviewer/evidence/origin-feature-after.txt @@ -0,0 +1,3 @@ +92e9f52 review: mechanical fixes +6dee8f6 feature +fd311b2 base diff --git a/examples/pr-reviewer/flows.json b/examples/pr-reviewer/flows.json new file mode 100644 index 000000000..e62af0353 --- /dev/null +++ b/examples/pr-reviewer/flows.json @@ -0,0 +1 @@ +{ "cli": "claude" } diff --git a/examples/pr-reviewer/package.json b/examples/pr-reviewer/package.json new file mode 100644 index 000000000..e61d97988 --- /dev/null +++ b/examples/pr-reviewer/package.json @@ -0,0 +1,9 @@ +{ + "name": "@relayflows/pr-reviewer-flow", + "private": true, + "type": "module", + "description": "The pr-reviewer example: the wepost PR reviewer as a v2 relayflow. `npm test` runs its node:test suite against the exported pure functions; `flows check pr-reviewer.flow.ts` preflights the flow. Marks the directory as ESM so node --experimental-strip-types runs it without reparsing.", + "scripts": { + "test": "node --experimental-strip-types --test tests/*.test.ts" + } +} diff --git a/examples/pr-reviewer/pr-reviewer.flow.ts b/examples/pr-reviewer/pr-reviewer.flow.ts new file mode 100644 index 000000000..7b3ebb0a5 --- /dev/null +++ b/examples/pr-reviewer/pr-reviewer.flow.ts @@ -0,0 +1,884 @@ +// pr-reviewer — the wepost-no/agents `review/agent.ts` PR reviewer, ported to +// a v2 relayflow (docs/SURFACE.md dialect). +// +// The v4 agent ran one harness prompt and relied on the surrounding platform +// for everything the prompt could not be trusted with: cloud materialized the +// checkout and `.workforce/pr.diff`, cloud committed and pushed whatever the +// agent left in the tree, and the prompt begged the agent not to lie about +// tests. Here every one of those becomes a journaled step the agent cannot +// forge: +// +// PR state → `f.run` + curl against the GitHub API, parsed in TS +// checkout + diff → `f.run` git commands +// the review → ONE `f.agent` step, gated on the review file existing +// verification → `f.run` runs the repo's tests OUTSIDE the agent; the +// exit code is the kernel's, so a red run cannot be +// talked into green +// push / comment / → deterministic `f.run` git and the journaled +// merge `f.github` helper effects (exactly-once receipts) +// +// The v4 harness-retry-on-OOM, exit-code diagnostics and log plumbing are not +// ported: the kernel owns retries and leases, and the journal is the log. +// +// One file on purpose. Cloud (`flows run --cloud --sync-code`, `flows deploy`) +// receives a single authored source and does not resolve sibling imports, so +// the pure functions live below the flow and are exported for the tests. +// +// TODO(flows#434 follow-up): once the `artifact_exists` named gate lands, +// replace the `subprocess_gate` on the review step with +// `.gate({ type: "artifact_exists", path: REVIEW_FILE })`. +// +// Not yet wired, deliberately: the `check_run.completed` (merge-on-green) and +// `issue_comment.created` (`@relay fix conflicts`) events are not in the +// generated trigger vocabulary (`@relayflows/surface/triggers/github` has +// issues, pull_request, pull_request_review, push). Their decision logic is +// ported (`evaluateMergeOnGreenState`, `matchesConflictDirective`, +// `isAuthorizedConflictCommander`) and unit-tested, but nothing dispatches it. + +import { flow, github } from "@relayflows/surface"; + +// ── input ─────────────────────────────────────────────────────────────────── + +export interface Input { + owner: string; + repo: string; + number: number; + /** Comma-separated GitHub logins whose approval merges the PR. Empty: any approval. */ + approvers: string; + /** Comma-separated logins to review. Empty: review everyone. */ + reviewAuthors?: string; + /** Comma-separated labels that turn the reviewer off. Default "no-agent-relay-review". */ + skipLabels?: string; + /** + * How GitHub writes reach GitHub. `helper` (default) is the journaled + * `f.github` effect with an exactly-once receipt; it needs a relayfile + * GitHub mount, which Cloud provides. `curl` posts through the REST API with + * `$GH_TOKEN` from a deterministic step, for a local checkout with no mount. + */ + githubTransport?: "helper" | "curl"; + /** The coding-agent CLI that writes the review. Default `claude`; `codex`, or a custom wrapper path. */ + reviewerCli?: string; + /** A GitHub webhook payload, when this run was launched by one. */ + event?: unknown; +} + +export const REVIEW_FILE = ".workforce/review.md"; +export const DEFAULT_SKIP_LABEL = "no-agent-relay-review"; + +// ── the flow ──────────────────────────────────────────────────────────────── + +const reviewer = flow( + "pr-reviewer", + { budget: { dollars: 8, wallclock: "45m" } }, + async (f, input) => { + const pr = prFromInput(input); + const api = (path: string) => + f.run(`curl -sf -H "Authorization: Bearer $GH_TOKEN" -H "Accept: application/vnd.github+json" ${shellWord(`https://api.github.com/repos/${pr.owner}/${pr.repo}${path}`)}`); + + // An approval from an allowlisted approver ends the loop: merge and stop. + if (input.event !== undefined && isApproval(input.event) && isAuthorizedApprover(input.approvers, input.event)) { + const merged = await mergePullRequest(f, input, pr); + // The journal is the log: record the outcome as a step, not stdout. + await f.run(`printf '%s\\n' ${shellWord(merged ? `merged #${pr.number}` : `GitHub did not confirm the merge of #${pr.number}`)}`); + return f.done(merged ? "success" : "step_failed"); + } + + // ── review gate: merged/closed, draft, disabling label, author allowlist ── + const meta = JSON.parse(await api(`/pulls/${pr.number}`)) as PrMeta; + const skip = shouldSkipReview(meta, pr, { skipLabels: input.skipLabels, reviewAuthors: input.reviewAuthors }); + if (skip) { + // No work is an outcome. Not `canceled`: that is a kernel fact a body + // cannot declare (flows#436), and a declination reason is still open. + await f.run(`printf '%s\\n' ${shellWord(`pr-reviewer skipped #${pr.number}: ${skip.reason}`)}`); + return f.done("success"); + } + const headRef = readString(meta.head?.ref) ?? `pull/${pr.number}/head`; + const baseRef = readString(meta.base?.ref) ?? "main"; + + // ── materialize what the agent reads: checkout at the PR head, the diff, + // the metadata. All deterministic, all journaled. ── + // The PR head goes into a named ref: `git checkout FETCH_HEAD` after a + // two-ref fetch silently takes the FIRST ref, which is the base branch. + await f.run(`git fetch --no-tags --depth=200 origin ${shellWord(`+refs/heads/${baseRef}:refs/remotes/origin/${baseRef}`)} ${shellWord(`+refs/pull/${pr.number}/head:refs/remotes/origin/pr-${pr.number}`)} && git checkout -q -B review ${shellWord(`origin/pr-${pr.number}`)}`, { timeout: "5m" }); + await f.run(`mkdir -p .workforce && git diff ${shellWord(`origin/${baseRef}`)}...HEAD > .workforce/pr.diff && git diff --name-only ${shellWord(`origin/${baseRef}`)}...HEAD > .workforce/changed-files.txt`); + await f.run(`printf '%s' ${shellWord(JSON.stringify(reviewContext(meta, pr)))} > .workforce/context.json`); + const comments = JSON.parse(await api(`/pulls/${pr.number}/comments?per_page=100`)) as unknown; + const reviews = JSON.parse(await api(`/pulls/${pr.number}/reviews?per_page=100`)) as unknown; + await f.run(`printf '%s' ${shellWord(JSON.stringify({ reviewComments: comments, reviews }))} > .workforce/threads.json`); + + // ── the review. One agent step, gated on the file it must write. ── + await f + .agent("review", { + cli: input.reviewerCli ?? "claude", + task: reviewHarnessPrompt(pr) + `\nWrite the review to ${REVIEW_FILE}. Read .workforce/threads.json for the existing bot and reviewer comments.`, + }) + // TODO: `.gate({ type: "artifact_exists", path: REVIEW_FILE })` once flows#434's follow-up lands. + .gate({ type: "subprocess_gate", command: `test -s ${REVIEW_FILE}` }); + + // ── verification, outside the agent. The exit code is the kernel's. ── + const verified = await f.run( + `if ${HARNESS_RESOURCE_ENV_SHELL} npm test > .workforce/test.log 2>&1; then echo PASS; else echo FAIL; fi`, + { timeout: "15m" }, + ); + // trimEnd before the sentinel check: `stripLastLine` on a trailing newline + // would strip only the newline and leave READY in the posted body. + let review = (await f.run(`cat ${REVIEW_FILE}`)).trimEnd(); + const harnessReady = lastLine(review) === "READY"; + if (harnessReady) review = stripLastLine(review).trimEnd(); + + // `.workforce/` is the flow's scratch, never the PR's: stage everything, + // then unstage it (an exclude pathspec exits 1 when the dir is gitignored). + const changed = (await f.run(`git add -A && git reset -q -- .workforce && git diff --cached --name-only`)).trim(); + if (changed && verified.trim() === "PASS") { + // Mechanical fixes, verified by the full test command, go to the PR. + await f.run(`git -c user.name=Relayflow -c user.email=noreply@agentrelay.com commit -q -m "review: mechanical fixes" && git push origin ${shellWord(`HEAD:refs/heads/${headRef}`)}`, { timeout: "5m" }); + } else if (changed) { + // An unverified push is worse than no push: discard, and say so. + await f.run(`git reset -q && git checkout -- . && git clean -fdq -e .workforce`); + review += "\n\n## Advisory\nThe reviewer's edits were discarded: the repository's test command was red in the review sandbox (see .workforce/test.log). The proposed changes are described above; nothing was pushed."; + } + + // Only call it a human's turn when it actually is: the agent said READY, + // the tests this flow ran were green (a signal the v4 agent never had), + // and GitHub's live state agrees. + const ready = harnessReady && verified.trim() === "PASS" && prReadyStateAllowsHumanReview(await readPrReviewState(api, pr)); + const body = ready ? `${review}\n\n:white_check_mark: This PR is ready for your review.` : review; + await postComment(f, input, pr, body); + f.done("success"); + }, +); + +// The trigger path, declared for when Cloud dispatches PR events to a flow +// (today it launches the default body with the issue-shaped input). Each +// handler reads the PR from the payload and runs the same body. +reviewer.on(github.pull_request("opened"), async (f) => { f.done("success"); }); +reviewer.on(github.pull_request("synchronize"), async (f) => { f.done("success"); }); +reviewer.on(github.pull_request_review({ action: "submitted" }), async (f) => { f.done("success"); }); + +export default reviewer; + +// ── GitHub writes ─────────────────────────────────────────────────────────── +// Kept outside the body on purpose: `flows check` discovers `f.github` by +// reading the body's source, and a checkout with no relayfile mount would be +// refused before running at all. Here the choice is made at run time by +// `githubTransport`, and the refusal (`helper_provider.mount_required`) +// arrives only if the helper is actually asked for without a mount. + +type Ctx = Parameters>[2]>[0]; + +async function postComment(f: Ctx, input: Input, pr: Pr, body: string): Promise { + if (input.githubTransport === "curl") { + await f.run(`curl -sf -X POST -H "Authorization: Bearer $GH_TOKEN" -H "Accept: application/vnd.github+json" ${shellWord(`https://api.github.com/repos/${pr.owner}/${pr.repo}/issues/${pr.number}/comments`)} -d ${shellWord(JSON.stringify({ body }))} > /dev/null`); + return; + } + await f.github.comment({ owner: pr.owner, repo: pr.repo, number: pr.number }, body); +} + +async function mergePullRequest(f: Ctx, input: Input, pr: Pr): Promise { + if (input.githubTransport === "curl") { + const out = await f.run(`curl -sf -X PUT -H "Authorization: Bearer $GH_TOKEN" -H "Accept: application/vnd.github+json" ${shellWord(`https://api.github.com/repos/${pr.owner}/${pr.repo}/pulls/${pr.number}/merge`)} -d ${shellWord(JSON.stringify({ merge_method: "squash", ...(pr.headSha ? { sha: pr.headSha } : {}) }))}`); + return (JSON.parse(out) as { merged?: unknown }).merged === true; + } + const result = await f.github.mergePullRequest({ + owner: pr.owner, repo: pr.repo, number: pr.number, method: "squash", ...(pr.headSha ? { sha: pr.headSha } : {}), + }); + return result.merged === true; +} + +// ── PR state from the REST API (the v4 agent read the relayfile VFS) ──────── + +async function readPrReviewState(api: (path: string) => PromiseLike, pr: Pr): Promise { + const meta = JSON.parse(await api(`/pulls/${pr.number}`)) as PrMeta; + const headSha = readString(meta.head?.sha); + const checks = headSha === undefined ? undefined + : JSON.parse(await api(`/commits/${headSha}/check-runs?per_page=100`)) as { check_runs?: unknown }; + const reviews = JSON.parse(await api(`/pulls/${pr.number}/reviews?per_page=100`)) as unknown; + if (headSha !== undefined) pr.headSha = headSha; + return prReviewStateFromRest(meta, checks, reviews); +} + +/** The same `PullRequestReadyState` the v4 gates consumed, built from REST responses. */ +export function prReviewStateFromRest(meta: PrMeta, checks: { check_runs?: unknown } | undefined, reviews: unknown): PullRequestReadyState { + const latestReviews = Array.isArray(reviews) ? reviews.filter((r): r is Record => r !== null && typeof r === "object") : []; + const checkRuns = Array.isArray(checks?.check_runs) ? checks.check_runs : []; + return { + state: typeof meta.state === "string" ? meta.state : undefined, + isDraft: meta.draft === true, + // REST reports mergeability directly (null while GitHub computes it). + mergeable: meta.mergeable === true ? "MERGEABLE" : meta.mergeable === false ? "CONFLICTING" : "UNKNOWN", + mergeStateStatus: meta.draft === true ? "DRAFT" + : typeof meta.mergeable_state === "string" ? meta.mergeable_state.toUpperCase() : undefined, + labels: meta.labels, + statusCheckRollup: checkRuns, + latestReviews, + reviewDecision: deriveReviewDecision(latestReviews), + ...(readString(meta.head?.sha) ? { headRefOid: meta.head!.sha } : {}), + }; +} + +// ── the pieces of the v4 agent that are pure, ported verbatim ─────────────── + +export interface Pr { + owner: string; + repo: string; + number: number; + url: string; + author: string; + headSha?: string; + state?: string; + merged?: boolean; + draft?: boolean; + labels?: unknown; +} + +/** The REST pull request record. Read defensively: fields may be absent. */ +export interface PrMeta { + state?: string; + merged?: boolean; + draft?: boolean; + mergeable?: boolean | null; + mergeable_state?: string; + author?: string | { login?: string }; + user?: { login?: string }; + labels?: unknown; + head?: { sha?: string; ref?: string }; + base?: { ref?: string }; + html_url?: string; + title?: string; + body?: string | null; + [key: string]: unknown; +} + +export const MERGE_ON_GREEN_LABEL = "merge-on-green"; +export const AGENT_WORKFORCE_ORG = "agentworkforce"; + +// The opt-in directive a commenter posts to ask for merge-conflict resolution. +// Accepts "@relay fix conflicts" / "@relay-bot resolve conflict" (case- and +// whitespace-insensitive). Deliberately narrow: it must be an explicit ask, not +// any mention, so an ordinary "there's a conflict here" comment never fires it. +const CONFLICT_DIRECTIVE_PATTERN = /@relay(?:-?bot)?\s+(?:fix|resolve)\s+conflicts?\b/i; + +/** + * Memory budget for the repo's own test command, sized against the sandbox + * (8 GiB hard ceiling; held to ONE worker so a 4 GiB V8 heap fits with + * headroom). Applied by the flow's `f.run` test step, not by the agent. + */ +export const HARNESS_RESOURCE_ENV: Record = { + NODE_OPTIONS: "--max-old-space-size=4096", + VITEST_MAX_THREADS: "1", + VITEST_MIN_THREADS: "1", + TURBO_CONCURRENCY: "1", + JEST_MAX_WORKERS: "1", +}; +const HARNESS_RESOURCE_ENV_SHELL = Object.entries(HARNESS_RESOURCE_ENV).map(([k, v]) => `${k}=${shellWord(v)}`).join(" "); + +export function prFromInput(input: Input): Pr { + const fromEvent = input.event === undefined ? undefined : readPr(input.event); + const owner = readString(input.owner) ?? fromEvent?.owner; + const repo = readString(input.repo) ?? fromEvent?.repo; + const number = Number.isInteger(input.number) ? input.number : fromEvent?.number; + if (!owner || !repo || number === undefined || !/^[A-Za-z0-9-]{1,39}$/.test(owner) || !/^[A-Za-z0-9_.-]{1,100}$/.test(repo)) { + throw new Error("pr-reviewer input needs owner, repo and an integer number (or a PR-shaped event)."); + } + return fromEvent && fromEvent.number === number ? fromEvent + : { owner, repo, number, url: `https://github.com/${owner}/${repo}/pull/${number}`, author: "unknown" }; +} + +function reviewContext(meta: PrMeta, pr: Pr): Record { + return { + owner: pr.owner, repo: pr.repo, number: pr.number, url: readString(meta.html_url) ?? pr.url, + title: meta.title, body: meta.body ?? "", author: resolveAuthorLogin(meta, pr), + labels: labelNames(meta.labels), head: meta.head, base: meta.base, draft: meta.draft === true, + }; +} + +// ── review gate ───────────────────────────────────────────────────────────── +// Decide whether to (re)review/fix this PR at all. Returns a skip reason, or +// null to proceed. Gates, in order: already-merged, draft, a disabling label, +// and an author allowlist. +export function shouldSkipReview( + meta: PrMeta | undefined, + pr: Pr, + config: { skipLabels?: string | undefined; reviewAuthors?: string | undefined }, +): { reason: string; notify?: boolean } | null { + // Already merged/closed by the time we got here — don't post a stale review + // on a finished PR. + const state = (meta?.state ?? pr.state ?? "").trim().toLowerCase(); + if (meta?.merged === true || pr.merged === true || state === "closed") { + return { reason: "PR is already merged/closed" }; + } + // A draft PR is held by its author — they aren't asking for review yet, and an + // auto-push into a work-in-progress branch is unwanted. + if (meta?.draft === true || pr.draft === true) { + return { reason: "PR is a draft" }; + } + // A disabling label turns the reviewer off entirely for this PR. + const skipLabels = skipLabelSet(config.skipLabels); + const prLabels = labelNames(Array.isArray(meta?.labels) ? meta.labels : pr.labels); + const hit = prLabels.find((name) => skipLabels.has(name)); + if (hit) { + return { reason: `PR carries the "${hit}" label` }; + } + // Author allowlist: when set, only review/fix PRs opened by those logins. + // Fail closed when configured: an unresolvable author is skipped, not guessed. + const allow = commaSet(config.reviewAuthors); + const author = resolveAuthorLogin(meta, pr); + return reviewAuthorAllowlistDecision(allow, author); +} + +/** Lowercased PR author login, preferring the authoritative record (string or + * `{ login }`, or REST's `user.login`) and falling back to the webhook payload. */ +export function resolveAuthorLogin(meta: PrMeta | undefined, pr: Pr): string { + const fromMeta = typeof meta?.author === "string" ? meta.author : meta?.author?.login ?? meta?.user?.login; + return (fromMeta ?? pr.author ?? "").trim().toLowerCase(); +} + +/** Lowercased label names that disable the reviewer. Defaults to + * "no-agent-relay-review" when unset. */ +export function skipLabelSet(raw: string | undefined): Set { + return commaSet(raw ?? DEFAULT_SKIP_LABEL); +} + +function commaSet(raw: string | undefined): Set { + return new Set((raw ?? "").split(",").map((s) => s.trim().toLowerCase()).filter(Boolean)); +} + +export function reviewAuthorAllowlistDecision( + allow: Set, + author: string, +): { reason: string; notify?: boolean } | null { + if (allow.size === 0) { + return null; + } + if (!author || author === "unknown") { + return { reason: "REVIEW_AUTHORS is set but the PR author could not be resolved", notify: true }; + } + if (!allow.has(author)) { + return { reason: `author @${author} is not in REVIEW_AUTHORS` }; + } + return null; +} + +export function labelNames(labels: unknown): string[] { + if (!Array.isArray(labels)) return []; + return labels + .map((l) => (l && typeof (l as { name?: unknown }).name === "string" ? (l as { name: string }).name.trim().toLowerCase() : "")) + .filter(Boolean); +} + +/** Keep the tail: a heap-exhaustion stack lands at the END of the output. */ +export function harnessOutputTail(value: unknown, limit = 4000): string | undefined { + if (typeof value !== "string") return undefined; + const trimmed = value.trim(); + if (!trimmed) return undefined; + return trimmed.length <= limit ? trimmed : trimmed.slice(-limit); +} + +export function reviewHarnessPrompt(pr: { owner: string; repo: string; number: number }): string { + return [ + `Review pull request #${pr.number} in ${pr.owner}/${pr.repo}. The PR code is checked out in the current directory.`, + `Focus on the actual PR changes: read .workforce/pr.diff first, then .workforce/changed-files.txt and .workforce/context.json.`, + `Use the checked-out repo to trace the impact of this diff across callers, types, tests, config, and related files.`, + `Flag and fix breakage even when the affected file is outside the changed-file set, but do not do an unrelated full-repo audit.`, + `Auto-edit only lint, formatting, spelling, typo, import-order, or other mechanical non-semantic changes.`, + `Do not auto-edit semantic or safety-critical logic. For behavior changes, architecture changes, and any reviewer`, + `request that needs human judgment, leave a clear suggestion or review comment instead of changing files.`, + `If the PR already has a human review or approval, switch to suggestion/comment-only for everything except`, + `obvious mechanical cleanup that cannot change runtime behavior.`, + `Resolve failing CI checks by editing the code only when the fix is mechanical and non-semantic. Don't use git or the gh CLI; the flow commits`, + `and pushes your file edits to the PR after this step, only if the repository's full test command passes. In your output, do not claim that fixes were pushed,`, + `a GitHub review was submitted, or CI was verified; those are later steps that the flow reports separately.`, + `Validate every finding — yours or another bot's — against the CURRENT checkout before editing: review comments`, + `are often stale (already fixed by a later push). Reproduce the problem in the code as it is now, or skip it.`, + `Make the smallest fix that addresses a demonstrated problem. Do not rewrite, restructure, or "harden" working`, + `code beyond what the finding requires.`, + `Never change semantic or safety defaults: do not turn fail-closed states into fail-open states such as`, + `"timeout", "pending", throw, or undefined becoming "acked", true, {}, or another success/default path; do not`, + `swap truthiness checks for presence checks; do not edit guard default values. If a reviewer asks for one of`, + `these changes, explain the risk in your review and leave the code unchanged.`, + `Never touch lifecycle, termination, reaper, in-flight, dispatch, broker ownership, or process-cleanup code. Those`, + `areas are safety-critical; raise findings as comments for a human-authored patch instead.`, + `Stay within this PR's purpose (.workforce/pr.diff is the change; use .workforce/context.json for available PR`, + `metadata). A reviewer suggestion that changes files or behavior unrelated to`, + `that purpose — refactoring a module`, + `the PR doesn't touch, renaming resources in an adapter the PR never edits, a cross-cutting "while you're here"`, + `cleanup — does NOT belong in this PR: record it as an advisory note under a "## Advisory Notes" heading in your review and leave the code unchanged.`, + `Folding an unrelated change into the PR is how you break an unrelated package's build; when in doubt, scope out.`, + `Account for every bot and reviewer comment explicitly in your output under an "## Addressed comments" heading:`, + `one bullet per comment naming the bot/reviewer and what they raised, followed by either the file:line where you`, + `fixed it (e.g. "fixed in src/foo.ts:42") or, if you did not change anything, a one-line reason (stale —`, + `already handled by a later commit, or invalid because ). This is how the comment authors and the human`, + `see that each thread was handled and exactly where, so be specific with the path and line; do not say a comment`, + `was addressed without pointing to the fix.`, + `Verify every edit before you finish, and verify it the way CI does — not just the unit test next to the file.`, + `Run the repo's canonical build and test command end to end (read package.json / turbo.json / the CI workflow to`, + `find what CI actually runs, focusing only on build/test/typecheck steps; install dependencies if needed) so you catch breakage DOWNSTREAM of the file you`, + `The sandbox is memory-constrained, so run those steps SERIALLY: do not raise worker/concurrency counts, and`, + `prefer a tool's single-worker flag (e.g. --maxWorkers=1, --concurrency=1) when it has one. A build or test run`, + `that is killed outright verifies nothing, so a slower serial run is strictly better than a parallel one that dies.`, + `edited. In a monorepo, editing one source file can break a generated/committed artifact (a catalog, lockfile,`, + `snapshot, or generated types) or a different package that imports it: when a finding makes you touch a source`, + `that feeds a generated file, regenerate that file with the repo's own generator and rebuild the packages that`, + `consume it. A green "tests for the file I touched" while the full build/test is red is exactly the failure that`, + `ships — the working tree must pass the full command with your edits in place. When you change code that`, + `GENERATES commands, scripts, or queries, also execute a sample of the generated output against a throwaway`, + `fixture — tests that only assert on the generated string prove nothing about its behavior.`, + `Never add or modify tests to make your own change pass. If a change needs a new or updated test, that is a`, + `human decision; describe the needed test in your review and leave the working tree unchanged.`, + `Never make a check pass by weakening the test: do not delete it, skip it, loosen an assertion, narrow its`, + `inputs, or replace a real assertion with a trivially-true one. A test that no longer fails when the behavior it`, + `guards regresses is worse than no test, and it passes CI while hiding the bug. When an edit makes a test fail,`, + `fix the CODE; only change a test's EXPECTATION when the test encoded the OLD, now-intentionally-changed contract`, + `and the new expected value is demonstrably correct — and say which in your "## Addressed comments" notes. If you`, + `cannot make a test genuinely pass, leave the code unfixed and raise it as advisory rather than gutting the test.`, + `If you cannot verify an edit (tests cannot run in this sandbox and you cannot make them run), do not leave it`, + `in the working tree: discard it with "git restore " — the one exception to the no-git rule, because`, + `rewriting a file back from memory is error-prone — delete files you created, and present the proposed change as`, + `advisory text in your review instead. Anything left in the working tree is committed and pushed to the PR after`, + `you exit, if the full test command passes — an unverified push is worse than no push.`, + `Only end your output with READY on its own last line when the PR genuinely needs a human now — meaning you have`, + `resolved or addressed every bot and reviewer comment, every required CI check has completed (none are pending`, + `or in-progress) and all are passing, the PR has no merge conflicts (GitHub reports it as mergeable), and the`, + `remaining decision requires human judgment. If any check is still pending, in-progress, or failed, or if the PR`, + `has merge conflicts, do NOT print READY.`, + ].join("\n"); +} + +export function conflictResolveHarnessPrompt(pr: { owner: string; repo: string; number: number }): string { + return [ + `Resolve the merge conflicts on pull request #${pr.number} in ${pr.owner}/${pr.repo}. The PR is checked out in the`, + `current directory and the flow has already merged the base branch into the working tree, so conflict markers`, + `(<<<<<<<, =======, >>>>>>>) are present in the conflicted files. Read .workforce/conflicted-files.txt for the`, + `exact list, and .workforce/pr.diff plus .workforce/context.json to understand what this PR changed versus base.`, + `Resolve EVERY conflict with the smallest correct merge that preserves BOTH sides' intent: understand what each`, + `side changed and why, then combine them so neither change is silently dropped. Do not blindly pick one side.`, + `Remove every conflict marker you resolve — leave no <<<<<<<, =======, or >>>>>>> behind in any file you touch.`, + `Do NOT use git or the gh CLI. The flow finalizes the merge commit and pushes it to the PR after you exit; your job`, + `is only to leave a correctly merged working tree. Do not claim the merge was committed or pushed — that is a`, + `later step the flow reports separately.`, + `Stay strictly within resolving the conflicts. Do not refactor, "harden", or fold in unrelated changes while`, + `merging — a conflict resolution that smuggles in extra edits is how an unrelated build breaks.`, + `Never resolve a conflict by changing a semantic or safety default: do not turn a fail-closed state into a`, + `fail-open one (a "timeout"/"pending"/throw/undefined becoming "acked"/true/{}/a success path), do not swap a`, + `truthiness check for a presence check, and do not alter guard default values to make the merge simpler.`, + `Never touch lifecycle, termination, reaper, in-flight, dispatch, broker ownership, or process-cleanup code to`, + `resolve a conflict; if the conflict is in one of those areas, treat it as needing human judgment (below).`, + `Never weaken or delete a test to resolve a conflict: do not skip it, loosen an assertion, or replace a real`, + `assertion with a trivially-true one. When both sides changed a test, merge both expectations honestly.`, + `If a conflict genuinely needs human judgment — the two sides are semantically incompatible, combining them is`, + `ambiguous or risky, or it lands in safety-critical code — do NOT guess. Leave that file's conflict markers in`, + `place, and list the file under a "## Unresolved conflicts" heading with a one-line reason. The flow aborts the`, + `merge and posts your explanation when any conflict is left unresolved, so a risky half-merge is never pushed.`, + `After resolving, verify the merged tree the way CI does — run the repo's canonical build/test/typecheck command`, + `end to end (read package.json / turbo.json / the CI workflow to find what CI runs; install dependencies if`, + `needed) so you catch breakage caused by combining the two sides, not just within one conflicted file. If you`, + `cannot make the merged tree pass and cannot fix it without human judgment, leave the remaining markers and`, + `record them under "## Unresolved conflicts" rather than pushing an unverified merge.`, + `Account for every conflicted file in your output: list each resolved file under a "## Resolved conflicts"`, + `heading with a one-line note on how you combined the two sides (e.g. "src/foo.ts — kept base's retry plus the`, + `PR's new timeout arg"), and list any you left for a human under "## Unresolved conflicts".`, + ].join("\n"); +} + +export interface PullRequestReadyState { + state?: unknown; + isDraft?: unknown; + labels?: unknown; + mergeable?: unknown; + mergeStateStatus?: unknown; + reviewDecision?: unknown; + reviewRequests?: unknown; + latestReviews?: unknown; + statusCheckRollup?: unknown; + headRefOid?: unknown; + url?: unknown; +} + +/** The adapter's per-PR aggregated check status (v4 VFS `checks/_summary.json`). */ +export interface CheckSummary { + total?: number; + passed?: number; + failed?: number; + pending?: number; + conclusion?: string; +} + +/** + * Turn the adapter's aggregated check counts into the statusCheckRollup shape + * the gate evaluators already understand. Kept for the trigger path, where a + * webhook payload carries counts rather than check runs. + */ +export function rollupFromCheckSummary(summary: CheckSummary | undefined): Array> { + const failed = typeof summary?.failed === "number" ? summary.failed : 0; + const pending = typeof summary?.pending === "number" ? summary.pending : 0; + const passed = typeof summary?.passed === "number" ? summary.passed : 0; + const total = typeof summary?.total === "number" ? summary.total : failed + pending + passed; + if (!summary || total === 0) return []; + const rollup: Array> = []; + if (failed > 0) { + rollup.push({ name: `${failed} failing check${failed === 1 ? "" : "s"}`, status: "COMPLETED", conclusion: "FAILURE" }); + } + if (pending > 0) { + rollup.push({ name: `${pending} pending check${pending === 1 ? "" : "s"}`, status: "IN_PROGRESS", conclusion: null }); + } + if (failed === 0 && pending === 0) { + rollup.push({ name: `${passed} check${passed === 1 ? "" : "s"}`, status: "COMPLETED", conclusion: "SUCCESS" }); + } + return rollup; +} + +/** + * Derive gh's `reviewDecision` from the reviews: if any author's latest review + * requested changes, the PR isn't the human's turn. + */ +export function deriveReviewDecision(reviews: Array>): string | undefined { + for (const [, review] of latestReviewsByAuthor(reviews)) { + if (normalizeState(review.state) === "CHANGES_REQUESTED") return "CHANGES_REQUESTED"; + } + return undefined; +} + +/** Whether the PR is still open. A missing `state` is treated as open. */ +function prIsOpen(state: PullRequestReadyState): boolean { + const s = normalizeState(state.state); + return s === undefined || s === "OPEN"; +} + +export function prReadyStateAllowsHumanReview(state: PullRequestReadyState): boolean { + // Never announce "ready for review" on a PR that merged or closed between the + // review step and this check. + if (!prIsOpen(state)) return false; + // A draft PR isn't up for review yet. + if (normalizeState(state.mergeStateStatus) === "DRAFT") return false; + // No merge conflicts. + if (state.mergeable !== "MERGEABLE") return false; + // A reviewer or bot still asking for changes means it isn't the human's turn. + if (normalizeState(state.reviewDecision) === "CHANGES_REQUESTED") return false; + // Checks must all be complete and passing. An *empty* rollup is ambiguous: + // only an empty rollup on a CLEAN PR counts as ready. + const checks = Array.isArray(state.statusCheckRollup) ? state.statusCheckRollup : []; + if (checks.length === 0) return normalizeState(state.mergeStateStatus) === "CLEAN"; + return checks.every(checkPassedAndComplete); +} + +export type MergeOnGreenOutcome = "merged" | "ready" | "blocked" | "pending" | "skipped"; + +export interface MergeOnGreenGate { + outcome: Exclude; + reasons: string[]; +} + +export function mergeOnGreenRepoAllowed(pr: Pr): boolean { + return pr.owner.trim().toLowerCase() === AGENT_WORKFORCE_ORG; +} + +export function evaluateMergeOnGreenState(state: PullRequestReadyState): MergeOnGreenGate { + const reasons: string[] = []; + if (!prIsOpen(state)) reasons.push(`PR is ${String(state.state ?? "not open")}`); + if (state.isDraft === true || normalizeState(state.mergeStateStatus) === "DRAFT") reasons.push("PR is a draft"); + if (!mergeOnGreenLabels(state.labels).has(MERGE_ON_GREEN_LABEL)) reasons.push(`PR is missing the "${MERGE_ON_GREEN_LABEL}" label`); + if (state.mergeable === "CONFLICTING") reasons.push("PR has merge conflicts"); + const checkReason = mergeOnGreenChecksReason(state); + if (checkReason) reasons.push(checkReason); + const reviewReason = mergeOnGreenBotReviewReason(state); + if (reviewReason) reasons.push(reviewReason); + + if (reasons.length === 0) return { outcome: "ready", reasons: [] }; + const blocked = reasons.some((reason) => + /fail|error|not passing|changes requested|requested changes|conflict|closed|merged|draft/i.test(reason), + ); + return { outcome: blocked ? "blocked" : "pending", reasons }; +} + +function mergeOnGreenLabels(labels: unknown): Set { + return new Set(labelNames(labels)); +} + +function mergeOnGreenChecksReason(state: PullRequestReadyState): string | null { + const checks = Array.isArray(state.statusCheckRollup) ? state.statusCheckRollup : []; + if (checks.length === 0) { + return normalizeState(state.mergeStateStatus) === "CLEAN" + ? null + : "checks are not reported as complete yet"; + } + const blocked = checks.find((check) => !checkPassedAndComplete(check)); + if (!blocked || typeof blocked !== "object") return null; + const record = blocked as Record; + const name = String(record.name ?? record.context ?? record.workflowName ?? "unknown"); + const stateText = String(record.state ?? record.status ?? "missing"); + const conclusionText = String(record.conclusion ?? "missing"); + if (stateText.toUpperCase() === "PENDING" || stateText.toUpperCase() === "IN_PROGRESS") { + return `check "${name}" is still ${stateText.toLowerCase()}`; + } + if (stateText.toUpperCase() !== "COMPLETED" && record.status !== undefined) { + return `check "${name}" is still ${stateText.toLowerCase()}`; + } + return `check "${name}" is not passing (${stateText}/${conclusionText})`; +} + +function mergeOnGreenBotReviewReason(state: PullRequestReadyState): string | null { + const latest = latestReviewsByAuthor(state.latestReviews); + for (const [login, review] of latest) { + const author = reviewAuthor(review); + if (!isBotLogin(author.login, author.type)) continue; + if (normalizeState(review.state) === "CHANGES_REQUESTED") { + return `bot @${login} requested changes`; + } + } + for (const login of requestedBotReviewLogins(state.reviewRequests)) { + const review = latest.get(login); + if (normalizeState(review?.state) !== "APPROVED") { + return `bot @${login} has not approved yet`; + } + } + return null; +} + +function requestedBotReviewLogins(reviewRequests: unknown): string[] { + if (!Array.isArray(reviewRequests)) return []; + const logins = new Set(); + for (const request of reviewRequests) { + if (!request || typeof request !== "object") continue; + const record = request as Record; + const reviewer = record.requestedReviewer && typeof record.requestedReviewer === "object" + ? record.requestedReviewer as Record + : record; + const login = readLogin(reviewer.login); + const type = typeof reviewer.type === "string" + ? reviewer.type + : typeof reviewer.__typename === "string" + ? reviewer.__typename + : undefined; + if (login && isBotLogin(login, type)) logins.add(login); + } + return [...logins].sort(); +} + +export function latestReviewsByAuthor(reviews: unknown): Map> { + const latest = new Map>(); + if (!Array.isArray(reviews)) return latest; + for (const review of reviews) { + if (!review || typeof review !== "object") continue; + const record = review as Record; + const author = reviewAuthor(record); + if (!author.login) continue; + const existing = latest.get(author.login); + if (!existing || reviewTimestamp(record) >= reviewTimestamp(existing)) { + latest.set(author.login, record); + } + } + return latest; +} + +function reviewAuthor(review: Record | undefined): { login: string; type?: string } { + if (!review) return { login: "" }; + const author = review.author && typeof review.author === "object" + ? review.author as Record + : review.user && typeof review.user === "object" + ? review.user as Record + : {}; + return { + login: readLogin(author.login), + type: typeof author.type === "string" + ? author.type + : typeof author.__typename === "string" + ? author.__typename + : undefined, + }; +} + +function reviewTimestamp(review: Record): number { + const date = typeof review.submittedAt === "string" + ? review.submittedAt + : typeof review.submitted_at === "string" + ? review.submitted_at + : ""; + const ms = Date.parse(date); + if (Number.isFinite(ms)) return ms; + return typeof review.id === "number" ? review.id : 0; +} + +function readLogin(value: unknown): string { + return typeof value === "string" ? value.trim().toLowerCase() : ""; +} + +function isBotLogin(login: string, type?: string): boolean { + return type === "Bot" || type === "BotUser" || login.endsWith("[bot]"); +} + +// A check that's complete and not blocking. SUCCESS and NEUTRAL pass; SKIPPED +// passes too — a conditionally-skipped job is GitHub's "not applicable". +function checkPassedAndComplete(check: unknown): boolean { + if (!check || typeof check !== "object") return false; + const record = check as Record; + const state = normalizeState(record.state); + if (state) return state === "SUCCESS" || state === "NEUTRAL" || state === "SKIPPED"; + const status = normalizeState(record.status); + const conclusion = normalizeState(record.conclusion); + return status === "COMPLETED" && (conclusion === "SUCCESS" || conclusion === "NEUTRAL" || conclusion === "SKIPPED"); +} + +function normalizeState(value: unknown): string | undefined { + return typeof value === "string" ? value.trim().toUpperCase() : undefined; +} + +export function describeNotReadyState(state: PullRequestReadyState): string { + if (!prIsOpen(state)) { + return `state=${String(state.state ?? "missing")}`; + } + if (normalizeState(state.mergeStateStatus) === "DRAFT") { + return "mergeStateStatus=DRAFT"; + } + if (state.mergeable !== "MERGEABLE") { + return `mergeable=${String(state.mergeable ?? "missing")}`; + } + if (normalizeState(state.reviewDecision) === "CHANGES_REQUESTED") { + return "reviewDecision=CHANGES_REQUESTED"; + } + const checks = Array.isArray(state.statusCheckRollup) ? state.statusCheckRollup : []; + if (checks.length === 0) { + return `no status checks reported and mergeStateStatus=${String(state.mergeStateStatus ?? "missing")} (not CLEAN)`; + } + const blocked = checks.find((check) => !checkPassedAndComplete(check)); + if (!blocked || typeof blocked !== "object") { + return "statusCheckRollup contains a non-passing check"; + } + const record = blocked as Record; + const name = record.name ?? record.context ?? record.workflowName ?? "unknown"; + const stateText = record.state ?? record.status ?? "missing"; + const conclusionText = record.conclusion ?? "missing"; + return `check=${String(name)} state=${String(stateText)} conclusion=${String(conclusionText)}`; +} + +// ── parsing the github webhook payload ────────────────────────────────────── +// The PR lives in different places per event: `pull_request` (opened / +// synchronize / review / review_comment), `check_run.pull_requests[0]` +// (check_run.completed), `issue` (issue_comment.created — the issue IS the PR +// when `issue.pull_request` is present), or the top-level `number`. +export function readPr(payload: unknown): Pr | undefined { + const p = payload as { + number?: number; + pull_request?: { + number?: number; + html_url?: string; + user?: { login?: string }; + head?: { sha?: string }; + state?: string; + merged?: boolean; + draft?: boolean; + labels?: unknown; + }; + issue?: { + number?: number; + html_url?: string; + user?: { login?: string }; + state?: string; + draft?: boolean; + labels?: unknown; + pull_request?: unknown; + }; + check_run?: { pull_requests?: Array<{ number?: number; html_url?: string; head_sha?: string }> }; + repository?: { name?: string; owner?: { login?: string } }; + sender?: { login?: string }; + } | null; + const prIssue = p?.issue?.pull_request != null ? p.issue : undefined; + const prRef = p?.pull_request ?? p?.check_run?.pull_requests?.[0] ?? prIssue; + const number = prRef?.number ?? p?.number; + const owner = p?.repository?.owner?.login; + const repo = p?.repository?.name; + if (typeof number !== "number" || !Number.isInteger(number) || !owner || !repo) return undefined; + const headSha = p?.pull_request?.head?.sha ?? p?.check_run?.pull_requests?.[0]?.head_sha; + const author = + p?.pull_request?.user?.login ?? + prIssue?.user?.login ?? + ((p?.pull_request || prIssue) ? p?.sender?.login : undefined) ?? + "unknown"; + const state = p?.pull_request?.state ?? prIssue?.state; + const draft = typeof p?.pull_request?.draft === "boolean" ? p.pull_request.draft : prIssue?.draft; + const labels = p?.pull_request?.labels ?? prIssue?.labels; + return { + owner, + repo, + number, + url: prRef?.html_url ?? `https://github.com/${owner}/${repo}/pull/${number}`, + author, + ...(headSha ? { headSha } : {}), + ...(state ? { state } : {}), + ...(typeof p?.pull_request?.merged === "boolean" ? { merged: p.pull_request.merged } : {}), + ...(typeof draft === "boolean" ? { draft } : {}), + ...(labels !== undefined ? { labels } : {}), + }; +} + +/** The body of an issue_comment webhook, or '' when absent. */ +export function commentBody(payload: unknown): string { + const body = (payload as { comment?: { body?: unknown } } | null)?.comment?.body; + return typeof body === "string" ? body : ""; +} + +/** The login of whoever wrote the comment (NOT the PR author), lowercased. */ +export function commenterLogin(payload: unknown): string { + const login = (payload as { comment?: { user?: { login?: unknown } } } | null)?.comment?.user?.login; + return typeof login === "string" ? login.trim().toLowerCase() : ""; +} + +/** Whether a comment body carries the opt-in conflict-resolution directive. */ +export function matchesConflictDirective(body: string): boolean { + return CONFLICT_DIRECTIVE_PATTERN.test(body); +} + +/** + * Who may command a conflict resolution. A bot never qualifies, and when + * approvers/reviewAuthors are configured the commenter must be the PR's own + * author or appear on one of those lists. With neither list set, anyone. + */ +export function isAuthorizedConflictCommander( + config: { approvers?: string | undefined; reviewAuthors?: string | undefined }, + commander: string, + pr: Pr, +): boolean { + if (!commander || commander.endsWith("[bot]")) return false; + const approvers = [...commaSet(config.approvers)]; + const reviewAuthors = commaSet(config.reviewAuthors); + if (approvers.length === 0 && reviewAuthors.size === 0) return true; + if (commander === (pr.author ?? "").trim().toLowerCase()) return true; + return approvers.includes(commander) || reviewAuthors.has(commander); +} + +export function isApproval(payload: unknown): boolean { + return (payload as { review?: { state?: string } } | null)?.review?.state?.toLowerCase() === "approved"; +} + +/** Honor approvals only from `approvers` (comma-separated logins). Unset: any approval merges. */ +export function isAuthorizedApprover(approvers: string | undefined, payload: unknown): boolean { + const allow = [...commaSet(approvers)]; + if (allow.length === 0) return true; + const approver = (payload as { review?: { user?: { login?: string } } } | null)?.review?.user?.login?.toLowerCase(); + return approver !== undefined && allow.includes(approver); +} + +/** A finished check run that didn't pass — failure, timed out, cancelled, etc. */ +export function ciFailed(payload: unknown): boolean { + const conclusion = (payload as { check_run?: { conclusion?: string } } | null)?.check_run?.conclusion?.toLowerCase(); + return conclusion !== undefined && conclusion !== "success" && conclusion !== "neutral" && conclusion !== "skipped"; +} + +// ── tiny helpers ──────────────────────────────────────────────────────────── + +export function lastLine(text: string): string { + return text.trimEnd().split("\n").pop()?.trim() ?? ""; +} + +export function stripLastLine(text: string): string { + const i = text.lastIndexOf("\n"); + return i < 0 ? "" : text.slice(0, i); +} + +function readString(value: unknown): string | undefined { + return typeof value === "string" && value.trim() ? value : undefined; +} + +/** Quote one shell word; the only way author strings reach a command. */ +export function shellWord(value: string): string { + return `'${value.replaceAll("'", "'\\''")}'`; +} diff --git a/examples/pr-reviewer/tests/pr-state.test.ts b/examples/pr-reviewer/tests/pr-state.test.ts new file mode 100644 index 000000000..26e6f6230 --- /dev/null +++ b/examples/pr-reviewer/tests/pr-state.test.ts @@ -0,0 +1,609 @@ +// Ported from wepost-no/agents tests/review-agent.test.mjs. The pure functions +// are the same; the `ctx`-reading ones take a config object here, and the v4 +// harness-retry / exit-code diagnostics tests are gone with the code they +// tested (the kernel owns retries and leases; the journal is the log). +// +// node --experimental-strip-types --test examples/pr-reviewer/tests/pr-state.test.ts + +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + commentBody, + commenterLogin, + HARNESS_RESOURCE_ENV, + conflictResolveHarnessPrompt, + harnessOutputTail, + deriveReviewDecision, + evaluateMergeOnGreenState, + isAuthorizedConflictCommander, + labelNames, + matchesConflictDirective, + prReadyStateAllowsHumanReview, + prReviewStateFromRest, + prFromInput, + readPr, + resolveAuthorLogin, + reviewHarnessPrompt, + reviewAuthorAllowlistDecision, + rollupFromCheckSummary, + shouldSkipReview, + shellWord, +} from "../pr-reviewer.flow.ts"; + +test('reviewAuthorAllowlistDecision lets configured authors through', () => { + assert.equal(reviewAuthorAllowlistDecision(new Set(['willwashburn']), 'willwashburn'), null); +}); + +test('reviewAuthorAllowlistDecision skips authors not in the allowlist', () => { + assert.deepEqual( + reviewAuthorAllowlistDecision(new Set(['khaliqgant']), 'willwashburn'), + { reason: 'author @willwashburn is not in REVIEW_AUTHORS' }, + ); +}); + +test('reviewAuthorAllowlistDecision skips unresolved authors when configured', () => { + assert.deepEqual( + reviewAuthorAllowlistDecision(new Set(['khaliqgant']), ''), + { reason: 'REVIEW_AUTHORS is set but the PR author could not be resolved', notify: true }, + ); + assert.deepEqual( + reviewAuthorAllowlistDecision(new Set(['khaliqgant']), 'unknown'), + { reason: 'REVIEW_AUTHORS is set but the PR author could not be resolved', notify: true }, + ); +}); + +test('reviewAuthorAllowlistDecision leaves unset allowlists open to everyone', () => { + assert.equal(reviewAuthorAllowlistDecision(new Set(), 'willwashburn'), null); + assert.equal(reviewAuthorAllowlistDecision(new Set(), ''), null); + assert.equal(reviewAuthorAllowlistDecision(new Set(), 'unknown'), null); +}); + +test('resolveAuthorLogin prefers normalized meta author shapes', () => { + assert.equal(resolveAuthorLogin({ author: ' WillWashburn ' }, { author: 'fallback' }), 'willwashburn'); + assert.equal(resolveAuthorLogin({ author: { login: ' KhaliqGant ' } }, { author: 'fallback' }), 'khaliqgant'); + assert.equal(resolveAuthorLogin({}, { author: ' FallBack ' }), 'fallback'); +}); + +test('readPr does not treat check-run sender as the PR author', () => { + assert.deepEqual(readPr({ + check_run: { + pull_requests: [{ + number: 27, + html_url: 'https://github.com/AgentWorkforce/agents/pull/27', + head_sha: 'abc123', + }], + }, + repository: { name: 'agents', owner: { login: 'AgentWorkforce' } }, + sender: { login: 'allowed-bot' }, + }), { + owner: 'AgentWorkforce', + repo: 'agents', + number: 27, + url: 'https://github.com/AgentWorkforce/agents/pull/27', + author: 'unknown', + headSha: 'abc123', + }); +}); + +test('readPr uses the pull request opener as author when present', () => { + assert.equal(readPr({ + number: 27, + pull_request: { + number: 27, + html_url: 'https://github.com/AgentWorkforce/agents/pull/27', + user: { login: 'WillWashburn' }, + }, + repository: { name: 'agents', owner: { login: 'AgentWorkforce' } }, + sender: { login: 'reviewer' }, + })?.author, 'WillWashburn'); +}); + +test('readPr falls back to sender login for PR-shaped payloads when opener login is missing', () => { + assert.equal(readPr({ + number: 27, + pull_request: { + number: 27, + html_url: 'https://github.com/AgentWorkforce/agents/pull/27', + }, + repository: { name: 'agents', owner: { login: 'AgentWorkforce' } }, + sender: { login: 'KhaliqGant' }, + })?.author, 'KhaliqGant'); +}); + +test('readPr surfaces the draft flag so the draft gate can hold off', () => { + // The draft flag feeds shouldSkipReview's preemptive draft gate — a held PR + // must not be auto-reviewed/pushed. Read it off the pull_request payload. + assert.equal(readPr({ + number: 27, + pull_request: { + number: 27, + html_url: 'https://github.com/AgentWorkforce/agents/pull/27', + user: { login: 'WillWashburn' }, + draft: true, + }, + repository: { name: 'agents', owner: { login: 'AgentWorkforce' } }, + })?.draft, true); + // A non-draft PR carries draft:false (not undefined) so the gate can tell + // "explicitly ready" from "unknown". + assert.equal(readPr({ + number: 28, + pull_request: { + number: 28, + html_url: 'https://github.com/AgentWorkforce/agents/pull/28', + user: { login: 'WillWashburn' }, + draft: false, + }, + repository: { name: 'agents', owner: { login: 'AgentWorkforce' } }, + })?.draft, false); +}); + +test('matchesConflictDirective fires only on an explicit fix/resolve-conflicts ask', () => { + assert.equal(matchesConflictDirective('@relay fix conflicts'), true); + assert.equal(matchesConflictDirective('hey @relay-bot RESOLVE conflict now'), true); + assert.equal(matchesConflictDirective('@relay resolve conflicts please'), true); + // The directive words must be adjacent — filler between them does not fire, + // so a force-update is never triggered by a loose mention. + assert.equal(matchesConflictDirective('@relay-bot please RESOLVE the conflict'), false); + // A passing mention or a plain observation must NOT trigger a force-update. + assert.equal(matchesConflictDirective('@relay this PR has a conflict'), false); + assert.equal(matchesConflictDirective('there is a merge conflict here'), false); + assert.equal(matchesConflictDirective(''), false); +}); + +test('commentBody / commenterLogin read the issue_comment payload defensively', () => { + const payload = { comment: { body: '@relay fix conflicts', user: { login: 'KhaliqGant' } } }; + assert.equal(commentBody(payload), '@relay fix conflicts'); + assert.equal(commenterLogin(payload), 'khaliqgant'); + assert.equal(commentBody({}), ''); + assert.equal(commenterLogin({}), ''); +}); + +test('readPr reads a PR from an issue_comment payload when the issue is a pull request', () => { + const pr = readPr({ + action: 'created', + issue: { + number: 77, + html_url: 'https://github.com/AgentWorkforce/agents/pull/77', + user: { login: 'WillWashburn' }, // the PR opener — must win as author + state: 'open', + pull_request: { url: 'https://api.github.com/.../pulls/77' }, + }, + comment: { body: '@relay fix conflicts', user: { login: 'KhaliqGant' } }, + repository: { name: 'agents', owner: { login: 'AgentWorkforce' } }, + sender: { login: 'KhaliqGant' }, + }); + assert.equal(pr?.number, 77); + assert.equal(pr?.author, 'WillWashburn'); + assert.equal(pr?.url, 'https://github.com/AgentWorkforce/agents/pull/77'); +}); + +test('readPr ignores an issue_comment on a plain issue (no pull_request marker)', () => { + assert.equal(readPr({ + action: 'created', + issue: { number: 5, html_url: 'https://github.com/AgentWorkforce/agents/issues/5' }, + comment: { body: '@relay fix conflicts' }, + repository: { name: 'agents', owner: { login: 'AgentWorkforce' } }, + }), undefined); +}); + +test('isAuthorizedConflictCommander never takes the order from a bot', () => { + const pr = { owner: 'AgentWorkforce', repo: 'agents', number: 1, author: 'someone' }; + assert.equal(isAuthorizedConflictCommander({}, 'relay-conflict-autofix[bot]', pr), false); + assert.equal(isAuthorizedConflictCommander({}, '', pr), false); +}); + +test('isAuthorizedConflictCommander is open when no trust lists are configured', () => { + const pr = { owner: 'AgentWorkforce', repo: 'agents', number: 1, author: 'willwashburn' }; + assert.equal(isAuthorizedConflictCommander({}, 'anyone', pr), true); +}); + +test('isAuthorizedConflictCommander gates on APPROVERS/REVIEW_AUTHORS and the PR author', () => { + const pr = { owner: 'AgentWorkforce', repo: 'agents', number: 1, author: 'WillWashburn' }; + // PR author may always fix their own PR's conflicts even if not on a list. + assert.equal(isAuthorizedConflictCommander({ approvers: 'khaliqgant' }, 'willwashburn', pr), true); + // A listed approver qualifies. + assert.equal(isAuthorizedConflictCommander({ approvers: 'khaliqgant' }, 'khaliqgant', pr), true); + // A REVIEW_AUTHORS member qualifies too. + assert.equal(isAuthorizedConflictCommander({ reviewAuthors: 'octocat' }, 'octocat', pr), true); + // A stranger, with a list configured, does not. + assert.equal(isAuthorizedConflictCommander({ approvers: 'khaliqgant' }, 'randuser', pr), false); +}); + +test('conflictResolveHarnessPrompt keeps the no-git boundary and the safety/escape-hatch rules', () => { + const prompt = conflictResolveHarnessPrompt({ owner: 'AgentWorkforce', repo: 'agents', number: 99 }); + // Reads cloud's merged tree + conflicted-file manifest. + assert.match(prompt, /\.workforce\/conflicted-files\.txt/); + assert.match(prompt, /the flow has already merged the base branch into the working tree/); + // No git in the harness; cloud finalizes + pushes the merge. + assert.match(prompt, /Do NOT use git or the gh CLI/); + assert.match(prompt, /The flow finalizes the merge commit and pushes/); + // Combine both sides; strip every marker. + assert.match(prompt, /preserves BOTH sides' intent/); + assert.match(prompt, /leave no <<<<<<<, =======, or >>>>>>> behind/); + // Same safety guardrails as review. + assert.match(prompt, /fail-closed state into a\s+fail-open one/); + assert.match(prompt, /Never weaken or delete a test/); + assert.match(prompt, /Never touch lifecycle, termination, reaper/); + // Human-judgment escape hatch → cloud aborts the merge. + assert.match(prompt, /## Unresolved conflicts/); + assert.match(prompt, /The flow aborts the\s+merge/); + // CI-deep verification of the merged tree. + assert.match(prompt, /verify the merged tree the way CI does/); +}); + +test('labelNames normalizes github label arrays defensively', () => { + assert.deepEqual(labelNames([ + { name: ' No-Agent-Relay-Review ' }, + { name: '' }, + { name: 42 }, + null, + { other: 'ignored' }, + ]), ['no-agent-relay-review']); + assert.deepEqual(labelNames(undefined), []); +}); + +test('readPr resolves issue labeled payloads for pull requests in any AgentWorkforce repo', () => { + assert.deepEqual(readPr({ + action: 'labeled', + label: { name: 'merge-on-green' }, + issue: { + number: 158, + html_url: 'https://github.com/AgentWorkforce/relayfile-adapters/issues/158', + pull_request: {}, + labels: [{ name: 'merge-on-green' }], + }, + repository: { name: 'relayfile-adapters', owner: { login: 'AgentWorkforce' } }, + }), { + owner: 'AgentWorkforce', + repo: 'relayfile-adapters', + number: 158, + url: 'https://github.com/AgentWorkforce/relayfile-adapters/issues/158', + author: 'unknown', + labels: [{ name: 'merge-on-green' }], + }); +}); + +test('evaluateMergeOnGreenState requires label, green checks, and requested bot approvals', () => { + const base = { + state: 'OPEN', + isDraft: false, + mergeable: 'MERGEABLE', + mergeStateStatus: 'CLEAN', + labels: [{ name: 'merge-on-green' }], + statusCheckRollup: [ + { __typename: 'CheckRun', name: 'unit', status: 'COMPLETED', conclusion: 'SUCCESS' }, + ], + reviewRequests: [ + { requestedReviewer: { login: 'coderabbitai[bot]', type: 'Bot' } }, + ], + latestReviews: [ + { author: { login: 'coderabbitai[bot]', type: 'Bot' }, state: 'APPROVED', submittedAt: '2026-06-10T00:00:00Z' }, + ], + }; + + assert.deepEqual(evaluateMergeOnGreenState(base), { outcome: 'ready', reasons: [] }); + + assert.equal(evaluateMergeOnGreenState({ + ...base, + statusCheckRollup: [ + { __typename: 'CheckRun', name: 'unit', status: 'IN_PROGRESS', conclusion: null }, + ], + }).outcome, 'pending'); + + assert.deepEqual(evaluateMergeOnGreenState({ + ...base, + latestReviews: [], + }), { + outcome: 'pending', + reasons: ['bot @coderabbitai[bot] has not approved yet'], + }); + + assert.equal(evaluateMergeOnGreenState({ + ...base, + latestReviews: [ + { author: { login: 'gemini-code-assist[bot]', type: 'Bot' }, state: 'CHANGES_REQUESTED', submittedAt: '2026-06-10T00:00:00Z' }, + ], + }).outcome, 'blocked'); +}); + +test('rollupFromCheckSummary maps the adapter check summary to gate-ready rollups', () => { + // No checks ingested yet (missing / total 0) → empty rollup. The gates then + // fall through to mergeStateStatus (which the VFS path never reports CLEAN), + // so the PR HOLDS instead of going green on absent CI. + assert.deepEqual(rollupFromCheckSummary(undefined), []); + assert.deepEqual(rollupFromCheckSummary({ total: 0, passed: 0, failed: 0, pending: 0 }), []); + + // All complete and passing → one SUCCESS entry the evaluators read as green. + const green = rollupFromCheckSummary({ total: 3, passed: 3, failed: 0, pending: 0 }); + assert.equal(evaluateMergeOnGreenState({ + state: 'OPEN', isDraft: false, mergeable: 'MERGEABLE', + labels: [{ name: 'merge-on-green' }], statusCheckRollup: green, + }).outcome, 'ready'); + + // A pending check → IN_PROGRESS → the merge-on-green gate stays pending. + const pending = rollupFromCheckSummary({ total: 2, passed: 1, failed: 0, pending: 1 }); + assert.equal(evaluateMergeOnGreenState({ + state: 'OPEN', isDraft: false, mergeable: 'MERGEABLE', + labels: [{ name: 'merge-on-green' }], statusCheckRollup: pending, + }).outcome, 'pending'); + + // A failing check → FAILURE → blocked. + const failing = rollupFromCheckSummary({ total: 2, passed: 1, failed: 1, pending: 0 }); + assert.equal(evaluateMergeOnGreenState({ + state: 'OPEN', isDraft: false, mergeable: 'MERGEABLE', + labels: [{ name: 'merge-on-green' }], statusCheckRollup: failing, + }).outcome, 'blocked'); + + // Green checks also satisfy the human-review ready gate. + assert.equal(prReadyStateAllowsHumanReview({ + state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: green, + }), true); + + // `total` missing but component counts present → derive total from the counts + // so a failing check still blocks (not treated as "no checks reported"). + assert.equal(evaluateMergeOnGreenState({ + state: 'OPEN', isDraft: false, mergeable: 'MERGEABLE', labels: [{ name: 'merge-on-green' }], + statusCheckRollup: rollupFromCheckSummary({ failed: 1, pending: 0, passed: 2 }), + }).outcome, 'blocked'); +}); + +test('deriveReviewDecision flags CHANGES_REQUESTED from the latest review per author', () => { + // No reviews → undefined (not blocking). + assert.equal(deriveReviewDecision([]), undefined); + + // A later APPROVED supersedes an earlier CHANGES_REQUESTED from the same author. + assert.equal(deriveReviewDecision([ + { author: { login: 'coderabbitai[bot]' }, state: 'CHANGES_REQUESTED', submitted_at: '2026-06-10T00:00:00Z' }, + { author: { login: 'coderabbitai[bot]' }, state: 'APPROVED', submitted_at: '2026-06-11T00:00:00Z' }, + ]), undefined); + + // An outstanding CHANGES_REQUESTED (the author's latest) blocks. + assert.equal(deriveReviewDecision([ + { author: { login: 'willwashburn' }, state: 'APPROVED', submitted_at: '2026-06-10T00:00:00Z' }, + { author: { login: 'coderabbitai[bot]' }, state: 'CHANGES_REQUESTED', submitted_at: '2026-06-11T00:00:00Z' }, + ]), 'CHANGES_REQUESTED'); +}); + +test('reviewHarnessPrompt forbids git except the explicit restore-only carve-out', () => { + const prompt = reviewHarnessPrompt({ owner: 'AgentWorkforce', repo: 'agents', number: 47 }); + assert.match(prompt, /Don't use git or the gh CLI/); + // "git restore " is deliberately permitted for discarding unverified + // edits (agents#47 review): rewriting a file back from memory is error-prone, + // a restore from HEAD is not. It must be framed as the exception... + assert.match(prompt, /git restore .*exception to the no-git rule/); + // ...and no destructive/state-mutating git verb may creep in. + assert.doesNotMatch(prompt, /\bgit\s+(checkout|reset|clean|commit|push|add|fetch|pull|rebase|merge|stash)\b/); +}); + +test('reviewHarnessPrompt keeps fixes within the PR scope and verifies CI-deep', () => { + const prompt = reviewHarnessPrompt({ owner: 'AgentWorkforce', repo: 'agents', number: 162 }); + // Scope discipline: out-of-scope reviewer suggestions become advisory notes, + // not edits folded into this PR (the dropbox/linear scope-creep that broke an + // unrelated build in agents#162's downstream relayfile-adapters PR). + assert.match(prompt, /Stay within this PR's purpose/); + assert.match(prompt, /use \.workforce\/context\.json for available PR\s+metadata/); + assert.match(prompt, /record it as an advisory note under a "## Advisory Notes" heading in your review and leave the code unchanged/); + // Verification must be CI-deep (full build/test), not just the touched file, + // and must regenerate generated/committed artifacts the edit feeds. + assert.match(prompt, /verify it the way CI does/); + assert.match(prompt, /canonical build and test command end to end/); + assert.match(prompt, /regenerate that file with the repo's own generator/); + assert.match(prompt, /the working tree must pass the full command with your edits in place/); + // Anti-hollow guard: don't make a check pass by gutting the test. + assert.match(prompt, /Never make a check pass by weakening the test/); + assert.match(prompt, /worse than no test/); + assert.match(prompt, /only change a test's EXPECTATION when the test encoded the OLD/); +}); + +test('reviewHarnessPrompt limits auto-edits to mechanical changes', () => { + const prompt = reviewHarnessPrompt({ owner: 'AgentWorkforce', repo: 'agents', number: 266 }); + assert.match(prompt, /Auto-edit only lint, formatting, spelling, typo, import-order, or other mechanical non-semantic changes/); + assert.match(prompt, /Do not auto-edit semantic or safety-critical logic/); + assert.match(prompt, /leave a clear suggestion or review comment instead of changing files/); + assert.match(prompt, /PR already has a human review or approval/); + assert.match(prompt, /suggestion\/comment-only/); +}); + +test('reviewHarnessPrompt forbids safety-default and lifecycle edits', () => { + const prompt = reviewHarnessPrompt({ owner: 'AgentWorkforce', repo: 'factory-sdk', number: 264 }); + assert.match(prompt, /Never change semantic or safety defaults/); + assert.match(prompt, /fail-closed states into fail-open states/); + assert.match(prompt, /"timeout", "pending", throw, or undefined becoming "acked", true, \{\}/); + assert.match(prompt, /swap truthiness checks for presence checks/); + assert.match(prompt, /guard default values/); + assert.match(prompt, /Never touch lifecycle, termination, reaper, in-flight, dispatch, broker ownership, or process-cleanup code/); +}); + +test('reviewHarnessPrompt forbids self-justifying test edits', () => { + const prompt = reviewHarnessPrompt({ owner: 'AgentWorkforce', repo: 'agents', number: 243 }); + assert.match(prompt, /Never add or modify tests to make your own change pass/); + assert.match(prompt, /If a change needs a new or updated test, that is a\s+human decision/); + assert.match(prompt, /describe the needed test in your review and leave the working tree unchanged/); +}); + +test('reviewHarnessPrompt only allows READY after checks complete, pass, and the PR is mergeable', () => { + const prompt = reviewHarnessPrompt({ owner: 'AgentWorkforce', repo: 'agents', number: 100 }); + assert.match(prompt, /every required CI check has completed/); + assert.match(prompt, /none are pending\s+or in-progress/); + assert.match(prompt, /all are passing/); + assert.match(prompt, /GitHub reports it as mergeable/); + assert.match(prompt, /If any check is still pending, in-progress, or failed, or if the PR\s+has merge conflicts, do NOT print READY/); + assert.doesNotMatch(prompt, /there are no failing checks left/); +}); + +test('prReadyStateAllowsHumanReview downgrades READY while a check is pending', () => { + assert.equal(prReadyStateAllowsHumanReview({ + mergeable: 'MERGEABLE', + statusCheckRollup: [ + { __typename: 'CheckRun', name: 'unit', status: 'COMPLETED', conclusion: 'SUCCESS' }, + { __typename: 'StatusContext', context: 'deploy-preview', state: 'PENDING' }, + ], + }), false); +}); + +test('prReadyStateAllowsHumanReview requires mergeable PRs with only completed passing checks', () => { + assert.equal(prReadyStateAllowsHumanReview({ + mergeable: 'MERGEABLE', + statusCheckRollup: [ + { __typename: 'CheckRun', name: 'unit', status: 'COMPLETED', conclusion: 'SUCCESS' }, + { __typename: 'StatusContext', context: 'lint', state: 'NEUTRAL' }, + ], + }), true); + + assert.equal(prReadyStateAllowsHumanReview({ + mergeable: 'CONFLICTING', + statusCheckRollup: [ + { __typename: 'CheckRun', name: 'unit', status: 'COMPLETED', conclusion: 'SUCCESS' }, + ], + }), false); +}); + +test('prReadyStateAllowsHumanReview never reports a merged or closed PR ready', () => { + const passingChecks = [{ __typename: 'CheckRun', name: 'unit', status: 'COMPLETED', conclusion: 'SUCCESS' }]; + assert.equal(prReadyStateAllowsHumanReview({ + state: 'MERGED', mergeable: 'MERGEABLE', statusCheckRollup: passingChecks, + }), false); + assert.equal(prReadyStateAllowsHumanReview({ + state: 'CLOSED', mergeable: 'MERGEABLE', statusCheckRollup: passingChecks, + }), false); + // An explicit OPEN state still passes when everything else is green. + assert.equal(prReadyStateAllowsHumanReview({ + state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: passingChecks, + }), true); +}); + +test('prReadyStateAllowsHumanReview treats an empty (not-yet-registered) check rollup as not ready', () => { + // Empty rollup + not CLEAN = checks queued but not yet registered → pending. + assert.equal(prReadyStateAllowsHumanReview({ + state: 'OPEN', mergeable: 'MERGEABLE', mergeStateStatus: 'BLOCKED', statusCheckRollup: [], + }), false); + assert.equal(prReadyStateAllowsHumanReview({ + state: 'OPEN', mergeable: 'MERGEABLE', mergeStateStatus: 'UNKNOWN', + }), false); + // No mergeStateStatus at all is also not-ready (can't confirm nothing's pending). + assert.equal(prReadyStateAllowsHumanReview({ + state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [], + }), false); +}); + +test('prReadyStateAllowsHumanReview allows a no-CI repo (empty rollup) only when GitHub reports CLEAN', () => { + assert.equal(prReadyStateAllowsHumanReview({ + state: 'OPEN', mergeable: 'MERGEABLE', mergeStateStatus: 'CLEAN', statusCheckRollup: [], + }), true); +}); + +test('prReadyStateAllowsHumanReview treats skipped checks as non-blocking', () => { + assert.equal(prReadyStateAllowsHumanReview({ + state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [ + { __typename: 'CheckRun', name: 'unit', status: 'COMPLETED', conclusion: 'SUCCESS' }, + { __typename: 'CheckRun', name: 'e2e-conditional', status: 'COMPLETED', conclusion: 'SKIPPED' }, + { __typename: 'StatusContext', context: 'optional-gate', state: 'SKIPPED' }, + ], + }), true); +}); + +test('prReadyStateAllowsHumanReview holds back drafts and changes-requested PRs', () => { + const passingChecks = [{ __typename: 'CheckRun', name: 'unit', status: 'COMPLETED', conclusion: 'SUCCESS' }]; + assert.equal(prReadyStateAllowsHumanReview({ + state: 'OPEN', mergeable: 'MERGEABLE', mergeStateStatus: 'DRAFT', statusCheckRollup: passingChecks, + }), false); + assert.equal(prReadyStateAllowsHumanReview({ + state: 'OPEN', mergeable: 'MERGEABLE', reviewDecision: 'CHANGES_REQUESTED', statusCheckRollup: passingChecks, + }), false); +}); + +test('reviewHarnessPrompt requires accounting for each bot/reviewer comment with a location', () => { + const prompt = reviewHarnessPrompt({ owner: 'AgentWorkforce', repo: 'agents', number: 7 }); + assert.match(prompt, /## Addressed comments/); + assert.match(prompt, /file:line where you/); + assert.match(prompt, /do not say a comment\s+was addressed without pointing to the fix/); +}); + + +test('reviewHarnessPrompt tells the harness to run build/test serially', () => { + const prompt = reviewHarnessPrompt({ owner: 'wepost-no', repo: 'wepost-saga', number: 5020 }); + assert.match(prompt, /memory-constrained, so run those steps SERIALLY/); + assert.match(prompt, /do not raise worker\/concurrency counts/); + assert.match(prompt, /Run the repo's canonical build and test command end to end/); +}); + +test('harnessOutputTail keeps the END of the output and drops empties', () => { + // An OOM stack is the LAST thing written, so head-truncation would discard + // exactly the evidence this exists to capture. + assert.equal(harnessOutputTail('abcdef', 3), 'def'); + assert.equal(harnessOutputTail('short'), 'short'); + assert.equal(harnessOutputTail(' '), undefined); + assert.equal(harnessOutputTail(undefined), undefined); +}); + + +// ── added for the v2 port ─────────────────────────────────────────────────── + +test("shouldSkipReview applies the four gates in order and returns null to proceed", () => { + const pr = { owner: "o", repo: "r", number: 1, url: "u", author: "octocat" }; + assert.deepEqual(shouldSkipReview({ state: "closed" }, pr, {}), { reason: "PR is already merged/closed" }); + assert.deepEqual(shouldSkipReview({ merged: true, state: "open" }, pr, {}), { reason: "PR is already merged/closed" }); + assert.deepEqual(shouldSkipReview({ state: "open", draft: true }, pr, {}), { reason: "PR is a draft" }); + assert.deepEqual(shouldSkipReview({ state: "open", labels: [{ name: "No-Agent-Relay-Review" }] }, pr, {}), + { reason: 'PR carries the "no-agent-relay-review" label' }); + assert.deepEqual(shouldSkipReview({ state: "open", labels: [{ name: "hold" }] }, pr, { skipLabels: "hold,wip" }), + { reason: 'PR carries the "hold" label' }); + assert.deepEqual(shouldSkipReview({ state: "open", user: { login: "Octocat" } }, pr, { reviewAuthors: "someone" }), + { reason: "author @octocat is not in REVIEW_AUTHORS" }); + assert.equal(shouldSkipReview({ state: "open", user: { login: "Octocat" } }, pr, { reviewAuthors: "octocat" }), null); + assert.equal(shouldSkipReview({ state: "open" }, pr, {}), null); +}); + +test("resolveAuthorLogin reads REST's user.login when meta.author is absent", () => { + const pr = { owner: "o", repo: "r", number: 1, url: "u", author: "fallback" }; + assert.equal(resolveAuthorLogin({ user: { login: " Octocat " } }, pr), "octocat"); + assert.equal(resolveAuthorLogin({}, pr), "fallback"); +}); + +test("prReviewStateFromRest maps a REST pull, check-runs and reviews onto the gate state", () => { + const state = prReviewStateFromRest( + { state: "open", draft: false, mergeable: true, mergeable_state: "clean", head: { sha: "abc" }, labels: [{ name: "x" }] }, + { check_runs: [{ name: "ci", status: "completed", conclusion: "success" }] }, + [{ state: "APPROVED", user: { login: "human" }, submitted_at: "2026-09-17T00:00:00Z" }], + ); + assert.equal(state.mergeable, "MERGEABLE"); + assert.equal(state.mergeStateStatus, "CLEAN"); + assert.equal(state.headRefOid, "abc"); + assert.equal(state.reviewDecision, undefined); + assert.equal(prReadyStateAllowsHumanReview(state), true); + // GitHub has not computed mergeability yet: not ready, never guessed. + const pending = prReviewStateFromRest({ state: "open", mergeable: null }, { check_runs: [] }, []); + assert.equal(pending.mergeable, "UNKNOWN"); + assert.equal(prReadyStateAllowsHumanReview(pending), false); + // A conflicting PR is CONFLICTING for the merge-on-green gate. + assert.equal(prReviewStateFromRest({ state: "open", mergeable: false }, undefined, []).mergeable, "CONFLICTING"); + // A draft shows as DRAFT even when REST reports a mergeable_state. + assert.equal(prReviewStateFromRest({ state: "open", draft: true, mergeable_state: "clean" }, undefined, []).mergeStateStatus, "DRAFT"); +}); + +test("prFromInput takes owner/repo/number, or a PR-shaped event, and refuses shell-unsafe coordinates", () => { + assert.deepEqual(prFromInput({ owner: "o", repo: "r", number: 3, approvers: "" }), + { owner: "o", repo: "r", number: 3, url: "https://github.com/o/r/pull/3", author: "unknown" }); + const fromEvent = prFromInput({ owner: "", repo: "", number: Number.NaN, approvers: "", event: { + pull_request: { number: 9, user: { login: "opener" }, head: { sha: "h" } }, + repository: { name: "r", owner: { login: "o" } }, + } }); + assert.equal(fromEvent.number, 9); + assert.equal(fromEvent.author, "opener"); + assert.equal(fromEvent.headSha, "h"); + assert.throws(() => prFromInput({ owner: "o;rm -rf", repo: "r", number: 1, approvers: "" }), /owner, repo and an integer number/); + assert.throws(() => prFromInput({ owner: "o", repo: "r", number: 1.5, approvers: "" }), /integer number/); +}); + +test("shellWord quotes one word, including embedded quotes", () => { + assert.equal(shellWord("plain"), "'plain'"); + assert.equal(shellWord("it's"), "'it'\\''s'"); +}); + +test("HARNESS_RESOURCE_ENV holds the test run to one worker under the sandbox heap cap", () => { + assert.equal(HARNESS_RESOURCE_ENV.VITEST_MAX_THREADS, "1"); + assert.equal(HARNESS_RESOURCE_ENV.JEST_MAX_WORKERS, "1"); + const heapMib = Number(/--max-old-space-size=(\d+)/.exec(HARNESS_RESOURCE_ENV.NODE_OPTIONS ?? "")?.[1]); + assert.ok(heapMib <= 8192 - 2048); +}); diff --git a/examples/tsconfig.json b/examples/tsconfig.json index a0acc16ec..367e367bd 100644 --- a/examples/tsconfig.json +++ b/examples/tsconfig.json @@ -1,5 +1,5 @@ { - "//": "OPT-IN typecheck for the examples authored directly against the real @relayflows/surface package (social-post-pipeline/, pr-review-pipeline/, dependency-upgrade-bot/). Not part of `npm test`. Run: cd packages/surface && npm run typecheck:examples. Mirrors regressions/tsconfig.json's path-alias approach.", + "//": "OPT-IN typecheck for the examples authored directly against the real @relayflows/surface package (social-post-pipeline/, pr-review-pipeline/, pr-reviewer/, dependency-upgrade-bot/). Not part of `npm test`. Run: cd packages/surface && npm run typecheck:examples. Mirrors regressions/tsconfig.json's path-alias approach.", "compilerOptions": { "target": "ES2022", "module": "ESNext", @@ -19,6 +19,7 @@ "include": [ "social-post-pipeline/*.ts", "pr-review-pipeline/*.ts", + "pr-reviewer/*.ts", "dependency-upgrade-bot/*.ts" ] } From 74e9d99cda49e8a268264e81532a5b66819d8463 Mon Sep 17 00:00:00 2001 From: Relayflow Lead Date: Thu, 17 Sep 2026 15:14:38 -0700 Subject: [PATCH 2/3] examples(pr-reviewer): address the first review round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `.on()` returns a new immutable handle; the three trigger handlers were registered on discarded copies. Chained, and the exported handle is asserted to carry three handlers (`flows.json` now registers `github`). - Fork PRs: `head.repo.full_name` is read; a head in another repository is never pushed to `origin` — fixes are posted as advisory instead. - Verification cannot be forged by the agent: `testCommand` is pinned from input before the agent runs (default `npm test`, never read from the checkout), and a deterministic protected-path check (package.json, lockfiles, test files/dirs, runner + tsconfig, .github/, Makefile, Cargo/ go.mod/pyproject) vetoes both the push and READY. Proven: an agent that rewrites the test script to `true` gets its edits discarded, an advisory naming package.json, and no ready line. - READY also reads legacy commit statuses (`/commits/{sha}/status`) and merges them into the rollup with the check runs. - Merging on approval requires the head SHA, the approval's `commit_id` to match it, and a green mergeable live state; the merge call always carries the SHA guard. An event that names a different PR than the configured coordinates is refused rather than trusted. - Non-npm repositories: `testCommand` input. Six more tests (50 total); all five proof paths re-run through the real kernel with a fresh daemon each (a lingering daemon keeps the environment of the first run — documented). Co-Authored-By: Claude Opus 5 (1M context) --- examples/pr-reviewer/README.md | 43 ++++- .../pr-reviewer/evidence/agent-wrapper.mjs | 2 +- .../pr-reviewer/evidence/api-calls-fork.txt | 22 +++ .../evidence/api-calls-protected.txt | 17 ++ .../pr-reviewer/evidence/api-calls-red.txt | 7 +- .../evidence/api-calls-success.txt | 1 + examples/pr-reviewer/evidence/curl-shim.sh | 3 +- .../evidence/origin-feature-after.txt | 2 +- examples/pr-reviewer/flows.json | 2 +- examples/pr-reviewer/pr-reviewer.flow.ts | 149 +++++++++++++++--- examples/pr-reviewer/tests/pr-state.test.ts | 62 ++++++++ 11 files changed, 276 insertions(+), 34 deletions(-) create mode 100644 examples/pr-reviewer/evidence/api-calls-fork.txt create mode 100644 examples/pr-reviewer/evidence/api-calls-protected.txt diff --git a/examples/pr-reviewer/README.md b/examples/pr-reviewer/README.md index 7ae6d1740..39f015d75 100644 --- a/examples/pr-reviewer/README.md +++ b/examples/pr-reviewer/README.md @@ -60,13 +60,41 @@ resolve sibling imports, so the helpers live below the flow. - `approvers` — logins whose approval event merges. Empty: any approval. - `reviewAuthors` — only review PRs by these logins. Empty: everyone. -- `reviewerCli` — `claude` (default), `codex`, or a custom wrapper path. +- `reviewerCli` — `claude` (default), `codex`, or a custom wrapper path. The + operator chooses it, as they choose the flow: `flows check` resolves and + probes it before anything runs, and a wrapper must identify itself with the + `relayflows-agent-cli-v1` handshake or is refused `cli_unsupported`. +- `testCommand` — the verification command, default `npm test`. Pinned from + input **before** the agent runs and never read from the checkout, so an + agent edit to `package.json` cannot redefine "green". - `githubTransport` — `helper` (default) uses the journaled `f.github` effect, which needs a relayfile GitHub mount (Cloud has one). `curl` posts through the REST API with `$GH_TOKEN` from a deterministic step, for a local checkout without a mount. - `event` — a GitHub webhook payload, when one launched the run. +## What the flow will and will not push + +Before any push, deterministically: the pinned test command was green; the +agent did not touch a **protected path** (`package.json`, lockfiles, test +files and directories, test-runner and TypeScript config, `.github/`, +`Makefile`, `Cargo.toml`/`go.mod`/`pyproject.toml` — the verification's own +inputs and the tests themselves, which are human-owned); and the PR head lives +in this repository (a fork's head is another repo, so fixes for fork PRs are +posted as advisory, not pushed). A protected-path edit also vetoes READY, +because a green run that came with a rewritten test script proves nothing. +Beyond that, "mechanical only" is the prompt's contract, as it was in v4 — +the review lists every changed path, and the push is a normal commit a human +can revert. + +Merging on an approval is the one irreversible step, so it needs: an approval +from an allowlisted approver (empty `approvers` = anyone, the upstream +default — set it), the approval's `commit_id` equal to the current head (or +no `commit_id` at all), the live state green and mergeable, and the merge +call carries that head SHA so GitHub refuses if the head moves in between. +A webhook payload may enrich the configured coordinates (author, head, labels) +but never redirect them: an event naming a different PR is refused. + ## Run it locally From a checkout of the repository the PR belongs to: @@ -106,15 +134,20 @@ bare scratch `origin` with `refs/pull/7/head`. | Path | Result | | --- | --- | -| happy: tests green | 17 steps `success`; review commit pushed to the PR branch ([origin-feature-after.txt](evidence/origin-feature-after.txt)); comment posted with the READY sentinel stripped and the `:white_check_mark:` line present ([api-calls-success.txt](evidence/api-calls-success.txt)) | -| red: `npm test` exits 1 | 14 steps `success`; PR branch untouched; edits discarded; advisory section posted; **no** ready line ([api-calls-red.txt](evidence/api-calls-red.txt)) | +| happy: tests green | 18 steps `success`; review commit pushed to the PR branch ([origin-feature-after.txt](evidence/origin-feature-after.txt)); comment posted with the READY sentinel stripped and the `:white_check_mark:` line present; check runs **and** commit statuses read ([api-calls-success.txt](evidence/api-calls-success.txt)) | +| red: `npm test` exits 1 | 15 steps `success`; PR branch untouched; edits discarded; advisory posted; **no** ready line ([api-calls-red.txt](evidence/api-calls-red.txt)) | +| fork PR (`head.repo` ≠ base) | 19 steps `success`; base repo's branch untouched; advisory names the fork ([api-calls-fork.txt](evidence/api-calls-fork.txt)) | +| protected path: the agent rewrites `package.json`'s test script so tests "pass" | 15 steps `success`; nothing pushed; advisory names `package.json`; **no** ready line despite the green run ([api-calls-protected.txt](evidence/api-calls-protected.txt)) | | draft PR | 3 steps `success`; one API read, no checkout, no agent, no comment ([api-calls-draft.txt](evidence/api-calls-draft.txt)) | -Three bugs the proof caught before anyone else could: `checkout FETCH_HEAD` +Bugs the proof caught before anyone else could: `checkout FETCH_HEAD` after a two-ref fetch silently took the base branch (now a named `refs/remotes/origin/pr-N`); `git add -A ':!.workforce'` exits 1 when the directory is gitignored (now `add -A` + `reset -- .workforce`); and the READY -sentinel survived into the posted body without a `trimEnd()`. +sentinel survived into the posted body without a `trimEnd()`. Review bots +then caught that `flow(...).on(...)` returns a *new* handle, so three +discarded `.on` calls registered nothing (now chained), and that a green run +with an agent-edited test script was being trusted (now vetoed). Not proven here, honestly: a real Claude session writing a real review (the wrapper stands in for it), the `helper` transport (needs a relayfile GitHub diff --git a/examples/pr-reviewer/evidence/agent-wrapper.mjs b/examples/pr-reviewer/evidence/agent-wrapper.mjs index 5b2ebed85..a7f8fd6fa 100755 --- a/examples/pr-reviewer/evidence/agent-wrapper.mjs +++ b/examples/pr-reviewer/evidence/agent-wrapper.mjs @@ -7,6 +7,6 @@ if (request) { if (!existsSync('.workforce/pr.diff')) { console.error('no diff materialized'); process.exit(3); } mkdirSync('.workforce', { recursive: true }); writeFileSync('.workforce/review.md', '## Review\nLooks fine.\n\n## Addressed comments\n- none\nREADY\n'); - writeFileSync('a.js', 'export const a = 2; // reviewed\n'); // a "mechanical" edit + writeFileSync('a.js', 'export const a = 2; // reviewed\n'); if (existsSync('../touch-pkg')) writeFileSync('package.json', '{"name":"proof","type":"module","scripts":{"test":"true"}}\n'); // a "mechanical" edit console.log('review-written'); process.exit(0); } diff --git a/examples/pr-reviewer/evidence/api-calls-fork.txt b/examples/pr-reviewer/evidence/api-calls-fork.txt new file mode 100644 index 000000000..24e65548d --- /dev/null +++ b/examples/pr-reviewer/evidence/api-calls-fork.txt @@ -0,0 +1,22 @@ +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/comments?per_page=100 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/reviews?per_page=100 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/commits/6dee8f6fa764906cf2c7039ff217a991a6eb315b/check-runs?per_page=100 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/commits/6dee8f6fa764906cf2c7039ff217a991a6eb315b/status +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/reviews?per_page=100 +-sf -X POST -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/issues/7/comments -d {"body":"## Review +Looks fine. + +## Addressed comments +- none + +## Advisory +The reviewer's edits were discarded because the PR head lives in someone/r, not in o/r, and this flow only pushes to its own repository. Nothing was pushed. What it proposed: + +``` +a.js | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) +``` + +:white_check_mark: This PR is ready for your review."} diff --git a/examples/pr-reviewer/evidence/api-calls-protected.txt b/examples/pr-reviewer/evidence/api-calls-protected.txt new file mode 100644 index 000000000..c8c58a33e --- /dev/null +++ b/examples/pr-reviewer/evidence/api-calls-protected.txt @@ -0,0 +1,17 @@ +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/comments?per_page=100 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/reviews?per_page=100 +-sf -X POST -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/issues/7/comments -d {"body":"## Review +Looks fine. + +## Addressed comments +- none + +## Advisory +The reviewer's edits were discarded because it touched package.json, which the reviewer may not change (tests, their runner configuration, lockfiles and CI are human-owned). Nothing was pushed. What it proposed: + +``` +a.js | 2 +- + package.json | 2 +- + 2 files changed, 2 insertions(+), 2 deletions(-) +```"} diff --git a/examples/pr-reviewer/evidence/api-calls-red.txt b/examples/pr-reviewer/evidence/api-calls-red.txt index b93130624..137ad3be8 100644 --- a/examples/pr-reviewer/evidence/api-calls-red.txt +++ b/examples/pr-reviewer/evidence/api-calls-red.txt @@ -8,4 +8,9 @@ Looks fine. - none ## Advisory -The reviewer's edits were discarded: the repository's test command was red in the review sandbox (see .workforce/test.log). The proposed changes are described above; nothing was pushed."} +The reviewer's edits were discarded because the repository's test command was red in the review sandbox (see .workforce/test.log). Nothing was pushed. What it proposed: + +``` +a.js | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) +```"} diff --git a/examples/pr-reviewer/evidence/api-calls-success.txt b/examples/pr-reviewer/evidence/api-calls-success.txt index 2605012e4..a71bc70a0 100644 --- a/examples/pr-reviewer/evidence/api-calls-success.txt +++ b/examples/pr-reviewer/evidence/api-calls-success.txt @@ -3,6 +3,7 @@ -sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/reviews?per_page=100 -sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7 -sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/commits/6dee8f6fa764906cf2c7039ff217a991a6eb315b/check-runs?per_page=100 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/commits/6dee8f6fa764906cf2c7039ff217a991a6eb315b/status -sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/reviews?per_page=100 -sf -X POST -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/issues/7/comments -d {"body":"## Review Looks fine. diff --git a/examples/pr-reviewer/evidence/curl-shim.sh b/examples/pr-reviewer/evidence/curl-shim.sh index d3d6e5c57..f310e1b6b 100755 --- a/examples/pr-reviewer/evidence/curl-shim.sh +++ b/examples/pr-reviewer/evidence/curl-shim.sh @@ -7,8 +7,9 @@ draft="${PRPROOF_DRAFT:-false}" case "$url" in */pulls/[0-9]*/comments*) printf '[]' ;; */pulls/[0-9]*/reviews*) printf '[]' ;; - */pulls/[0-9]*) printf '{"state":"open","draft":%s,"merged":false,"mergeable":true,"mergeable_state":"clean","user":{"login":"octocat"},"labels":[],"head":{"sha":"%s","ref":"%s"},"base":{"ref":"main"},"html_url":"https://github.com/o/r/pull/7","title":"feature"}' "$draft" "$PRPROOF_HEAD" "$PRPROOF_REF" ;; + */pulls/[0-9]*) printf '{"state":"open","draft":%s,"merged":false,"mergeable":true,"mergeable_state":"clean","user":{"login":"octocat"},"labels":[],"head":{"sha":"%s","ref":"%s","repo":{"full_name":"%s"}},"base":{"ref":"main"},"html_url":"https://github.com/o/r/pull/7","title":"feature"}' "$draft" "$PRPROOF_HEAD" "$PRPROOF_REF" "${PRPROOF_HEAD_REPO:-o/r}" ;; */commits/*/check-runs*) printf '{"check_runs":[{"name":"ci","status":"completed","conclusion":"success"}]}' ;; + */commits/*/status) printf '{"state":"success","statuses":[]}' ;; */issues/[0-9]*/comments) printf '{"id":1}' ;; *) echo "unexpected $url" >&2; exit 22 ;; esac diff --git a/examples/pr-reviewer/evidence/origin-feature-after.txt b/examples/pr-reviewer/evidence/origin-feature-after.txt index 4ee4cabbe..6f9ff32f1 100644 --- a/examples/pr-reviewer/evidence/origin-feature-after.txt +++ b/examples/pr-reviewer/evidence/origin-feature-after.txt @@ -1,3 +1,3 @@ -92e9f52 review: mechanical fixes +2184542 review: mechanical fixes 6dee8f6 feature fd311b2 base diff --git a/examples/pr-reviewer/flows.json b/examples/pr-reviewer/flows.json index e62af0353..070245f47 100644 --- a/examples/pr-reviewer/flows.json +++ b/examples/pr-reviewer/flows.json @@ -1 +1 @@ -{ "cli": "claude" } +{ "cli": "claude", "executors": ["github"] } diff --git a/examples/pr-reviewer/pr-reviewer.flow.ts b/examples/pr-reviewer/pr-reviewer.flow.ts index 7b3ebb0a5..54faf92d9 100644 --- a/examples/pr-reviewer/pr-reviewer.flow.ts +++ b/examples/pr-reviewer/pr-reviewer.flow.ts @@ -58,6 +58,12 @@ export interface Input { githubTransport?: "helper" | "curl"; /** The coding-agent CLI that writes the review. Default `claude`; `codex`, or a custom wrapper path. */ reviewerCli?: string; + /** + * The repository's verification command, pinned by the operator BEFORE the + * agent runs. Default `npm test`. It is never read from the checkout, so an + * agent edit to package.json cannot redefine what "green" means. + */ + testCommand?: string; /** A GitHub webhook payload, when this run was launched by one. */ event?: unknown; } @@ -67,7 +73,7 @@ export const DEFAULT_SKIP_LABEL = "no-agent-relay-review"; // ── the flow ──────────────────────────────────────────────────────────────── -const reviewer = flow( +const reviewerBody = flow( "pr-reviewer", { budget: { dollars: 8, wallclock: "45m" } }, async (f, input) => { @@ -75,11 +81,19 @@ const reviewer = flow( const api = (path: string) => f.run(`curl -sf -H "Authorization: Bearer $GH_TOKEN" -H "Accept: application/vnd.github+json" ${shellWord(`https://api.github.com/repos/${pr.owner}/${pr.repo}${path}`)}`); - // An approval from an allowlisted approver ends the loop: merge and stop. + // An approval from an allowlisted approver ends the loop: merge and stop — + // but only the head that was approved, and only when the PR is green and + // mergeable right now. Merging is the one irreversible step here. if (input.event !== undefined && isApproval(input.event) && isAuthorizedApprover(input.approvers, input.event)) { + const state = await readPrReviewState(api, pr); + const refusal = mergeRefusal(state, pr, approvedCommit(input.event)); + if (refusal) { + await f.run(`printf '%s\\n' ${shellWord(`pr-reviewer did not merge #${pr.number}: ${refusal}`)}`); + return f.done("success"); + } const merged = await mergePullRequest(f, input, pr); // The journal is the log: record the outcome as a step, not stdout. - await f.run(`printf '%s\\n' ${shellWord(merged ? `merged #${pr.number}` : `GitHub did not confirm the merge of #${pr.number}`)}`); + await f.run(`printf '%s\\n' ${shellWord(merged ? `merged #${pr.number} at ${pr.headSha}` : `GitHub did not confirm the merge of #${pr.number}`)}`); return f.done(merged ? "success" : "step_failed"); } @@ -94,6 +108,10 @@ const reviewer = flow( } const headRef = readString(meta.head?.ref) ?? `pull/${pr.number}/head`; const baseRef = readString(meta.base?.ref) ?? "main"; + // A fork's head lives in another repository; `git push origin` would write + // a stray branch on the base repo. Fixes for fork PRs are posted, not pushed. + const headRepo = readString(meta.head?.repo?.full_name)?.toLowerCase(); + const sameRepo = headRepo === undefined || headRepo === `${pr.owner}/${pr.repo}`.toLowerCase(); // ── materialize what the agent reads: checkout at the PR head, the diff, // the metadata. All deterministic, all journaled. ── @@ -117,7 +135,7 @@ const reviewer = flow( // ── verification, outside the agent. The exit code is the kernel's. ── const verified = await f.run( - `if ${HARNESS_RESOURCE_ENV_SHELL} npm test > .workforce/test.log 2>&1; then echo PASS; else echo FAIL; fi`, + `if ${HARNESS_RESOURCE_ENV_SHELL} ${testCommand(input)} > .workforce/test.log 2>&1; then echo PASS; else echo FAIL; fi`, { timeout: "15m" }, ); // trimEnd before the sentinel check: `stripLastLine` on a trailing newline @@ -129,19 +147,36 @@ const reviewer = flow( // `.workforce/` is the flow's scratch, never the PR's: stage everything, // then unstage it (an exclude pathspec exits 1 when the dir is gitignored). const changed = (await f.run(`git add -A && git reset -q -- .workforce && git diff --cached --name-only`)).trim(); - if (changed && verified.trim() === "PASS") { - // Mechanical fixes, verified by the full test command, go to the PR. + const changedPaths = changed ? changed.split("\n") : []; + // A green run is evidence only if the agent left the verification's own + // inputs alone: a rewritten test script or test file can make anything + // pass. Protected paths therefore veto both the push and READY. + const protectedHit = protectedPathRefusal(changedPaths); + const trustedGreen = verified.trim() === "PASS" && protectedHit === undefined; + // Deterministic, before anything is pushed: the tests were green and + // trustworthy, the PR head is in this repository. "Mechanical only" + // beyond that is the prompt's contract, and the review says what was + // changed either way. + const pushRefusal = changedPaths.length === 0 ? undefined + : protectedHit !== undefined ? protectedHit + : verified.trim() !== "PASS" ? "the repository's test command was red in the review sandbox (see .workforce/test.log)" + : !sameRepo ? `the PR head lives in ${headRepo}, not in ${pr.owner}/${pr.repo}, and this flow only pushes to its own repository` + : undefined; + if (changedPaths.length > 0 && pushRefusal === undefined) { + // Mechanical fixes, verified by the pinned test command, go to the PR. await f.run(`git -c user.name=Relayflow -c user.email=noreply@agentrelay.com commit -q -m "review: mechanical fixes" && git push origin ${shellWord(`HEAD:refs/heads/${headRef}`)}`, { timeout: "5m" }); - } else if (changed) { - // An unverified push is worse than no push: discard, and say so. + } else if (changedPaths.length > 0) { + // An unverified or out-of-bounds push is worse than no push: keep the + // diff for the review, discard the edits, and say so. + const proposed = await f.run(`git diff --cached --stat | tail -n 20`); await f.run(`git reset -q && git checkout -- . && git clean -fdq -e .workforce`); - review += "\n\n## Advisory\nThe reviewer's edits were discarded: the repository's test command was red in the review sandbox (see .workforce/test.log). The proposed changes are described above; nothing was pushed."; + review += `\n\n## Advisory\nThe reviewer's edits were discarded because ${pushRefusal}. Nothing was pushed. What it proposed:\n\n\`\`\`\n${proposed.trim()}\n\`\`\``; } // Only call it a human's turn when it actually is: the agent said READY, // the tests this flow ran were green (a signal the v4 agent never had), // and GitHub's live state agrees. - const ready = harnessReady && verified.trim() === "PASS" && prReadyStateAllowsHumanReview(await readPrReviewState(api, pr)); + const ready = harnessReady && trustedGreen && prReadyStateAllowsHumanReview(await readPrReviewState(api, pr)); const body = ready ? `${review}\n\n:white_check_mark: This PR is ready for your review.` : review; await postComment(f, input, pr, body); f.done("success"); @@ -149,12 +184,16 @@ const reviewer = flow( ); // The trigger path, declared for when Cloud dispatches PR events to a flow -// (today it launches the default body with the issue-shaped input). Each -// handler reads the PR from the payload and runs the same body. -reviewer.on(github.pull_request("opened"), async (f) => { f.done("success"); }); -reviewer.on(github.pull_request("synchronize"), async (f) => { f.done("success"); }); -reviewer.on(github.pull_request_review({ action: "submitted" }), async (f) => { f.done("success"); }); - +// (today it launches the default body with the issue-shaped input). Handles +// are immutable — each `.on` returns a new one — so the chained result is what +// gets exported. Each handler currently acknowledges; when dispatch lands +// they read the PR from the payload and run the same body. +const reviewer = reviewerBody + .on(github.pull_request("opened"), async (f) => { f.done("success"); }) + .on(github.pull_request("synchronize"), async (f) => { f.done("success"); }) + .on(github.pull_request_review({ action: "submitted" }), async (f) => { f.done("success"); }); + +export { reviewer }; export default reviewer; // ── GitHub writes ─────────────────────────────────────────────────────────── @@ -176,15 +215,35 @@ async function postComment(f: Ctx, input: Input, pr: Pr, body: string): Promise< async function mergePullRequest(f: Ctx, input: Input, pr: Pr): Promise { if (input.githubTransport === "curl") { - const out = await f.run(`curl -sf -X PUT -H "Authorization: Bearer $GH_TOKEN" -H "Accept: application/vnd.github+json" ${shellWord(`https://api.github.com/repos/${pr.owner}/${pr.repo}/pulls/${pr.number}/merge`)} -d ${shellWord(JSON.stringify({ merge_method: "squash", ...(pr.headSha ? { sha: pr.headSha } : {}) }))}`); + const out = await f.run(`curl -sf -X PUT -H "Authorization: Bearer $GH_TOKEN" -H "Accept: application/vnd.github+json" ${shellWord(`https://api.github.com/repos/${pr.owner}/${pr.repo}/pulls/${pr.number}/merge`)} -d ${shellWord(JSON.stringify({ merge_method: "squash", sha: pr.headSha }))}`); return (JSON.parse(out) as { merged?: unknown }).merged === true; } + // `mergeRefusal` has already established `pr.headSha`; the SHA guard makes + // GitHub refuse if the head moved between the check and the merge. const result = await f.github.mergePullRequest({ - owner: pr.owner, repo: pr.repo, number: pr.number, method: "squash", ...(pr.headSha ? { sha: pr.headSha } : {}), + owner: pr.owner, repo: pr.repo, number: pr.number, method: "squash", sha: pr.headSha!, }); return result.merged === true; } +/** Why an approval must not merge right now, or undefined to proceed. */ +export function mergeRefusal(state: PullRequestReadyState, pr: Pr, approvedSha: string | undefined): string | undefined { + const headSha = readString(state.headRefOid); + if (headSha === undefined) return "the PR head SHA could not be read"; + if (approvedSha !== undefined && approvedSha !== headSha) { + return `the approval was for ${approvedSha}, but the head is now ${headSha}`; + } + if (!prReadyStateAllowsHumanReview(state)) return describeNotReadyState(state); + pr.headSha = headSha; + return undefined; +} + +/** The commit an approval review was submitted against. */ +export function approvedCommit(payload: unknown): string | undefined { + const sha = (payload as { review?: { commit_id?: unknown } } | null)?.review?.commit_id; + return typeof sha === "string" && /^[a-f0-9]{7,40}$/.test(sha) ? sha : undefined; +} + // ── PR state from the REST API (the v4 agent read the relayfile VFS) ──────── async function readPrReviewState(api: (path: string) => PromiseLike, pr: Pr): Promise { @@ -192,15 +251,27 @@ async function readPrReviewState(api: (path: string) => PromiseLike, pr: const headSha = readString(meta.head?.sha); const checks = headSha === undefined ? undefined : JSON.parse(await api(`/commits/${headSha}/check-runs?per_page=100`)) as { check_runs?: unknown }; + // Legacy commit statuses (deploy previews, external CI) are a separate + // endpoint from check runs; READY must see both. + const statuses = headSha === undefined ? undefined + : JSON.parse(await api(`/commits/${headSha}/status`)) as { statuses?: unknown }; const reviews = JSON.parse(await api(`/pulls/${pr.number}/reviews?per_page=100`)) as unknown; if (headSha !== undefined) pr.headSha = headSha; - return prReviewStateFromRest(meta, checks, reviews); + return prReviewStateFromRest(meta, checks, reviews, statuses); } /** The same `PullRequestReadyState` the v4 gates consumed, built from REST responses. */ -export function prReviewStateFromRest(meta: PrMeta, checks: { check_runs?: unknown } | undefined, reviews: unknown): PullRequestReadyState { +export function prReviewStateFromRest( + meta: PrMeta, checks: { check_runs?: unknown } | undefined, reviews: unknown, statuses?: { statuses?: unknown }, +): PullRequestReadyState { const latestReviews = Array.isArray(reviews) ? reviews.filter((r): r is Record => r !== null && typeof r === "object") : []; - const checkRuns = Array.isArray(checks?.check_runs) ? checks.check_runs : []; + const checkRuns = [ + ...(Array.isArray(checks?.check_runs) ? checks.check_runs : []), + // A status context has `state` (pending/success/failure/error) and no + // `status`/`conclusion`; `checkPassedAndComplete` reads `state` first. + ...(Array.isArray(statuses?.statuses) ? statuses.statuses.map((s) => + s !== null && typeof s === "object" ? { name: (s as { context?: unknown }).context, state: (s as { state?: unknown }).state } : s) : []), + ]; return { state: typeof meta.state === "string" ? meta.state : undefined, isDraft: meta.draft === true, @@ -241,7 +312,7 @@ export interface PrMeta { author?: string | { login?: string }; user?: { login?: string }; labels?: unknown; - head?: { sha?: string; ref?: string }; + head?: { sha?: string; ref?: string; repo?: { full_name?: string } | null }; base?: { ref?: string }; html_url?: string; title?: string; @@ -280,8 +351,38 @@ export function prFromInput(input: Input): Pr { if (!owner || !repo || number === undefined || !/^[A-Za-z0-9-]{1,39}$/.test(owner) || !/^[A-Za-z0-9_.-]{1,100}$/.test(repo)) { throw new Error("pr-reviewer input needs owner, repo and an integer number (or a PR-shaped event)."); } - return fromEvent && fromEvent.number === number ? fromEvent - : { owner, repo, number, url: `https://github.com/${owner}/${repo}/pull/${number}`, author: "unknown" }; + // The configured coordinates are the authority. A payload may enrich them + // (author, head SHA, labels) but never redirect them: the same event shape + // that launches a run could otherwise point a merge at another repository. + if (fromEvent && (fromEvent.owner.toLowerCase() !== owner.toLowerCase() || fromEvent.repo.toLowerCase() !== repo.toLowerCase() || fromEvent.number !== number)) { + throw new Error(`pr-reviewer event names ${fromEvent.owner}/${fromEvent.repo}#${fromEvent.number}, not the configured ${owner}/${repo}#${number}.`); + } + return fromEvent ?? { owner, repo, number, url: `https://github.com/${owner}/${repo}/pull/${number}`, author: "unknown" }; +} + +/** The pinned verification command, quoted for `/bin/sh -c`. Never read from the checkout. */ +export function testCommand(input: Pick): string { + const command = input.testCommand?.trim() || "npm test"; + if (command.includes("\0") || command.includes("\n")) throw new Error("testCommand must be a single shell line."); + return command; +} + +/** + * Paths the reviewer may never push, whatever the tests said: the test + * command's own inputs and the tests themselves (an edit there could be what + * made the run green), lockfiles, and CI/workflow configuration. + */ +export const PROTECTED_PATHS: readonly RegExp[] = [ + /(^|\/)package\.json$/, /(^|\/)package-lock\.json$/, /(^|\/)yarn\.lock$/, /(^|\/)pnpm-lock\.yaml$/, + /(^|\/)Cargo\.(toml|lock)$/, /(^|\/)go\.(mod|sum)$/, /(^|\/)pyproject\.toml$/, /(^|\/)requirements[^/]*\.txt$/, + /(^|\/)turbo\.json$/, /(^|\/)(vitest|jest|playwright)\.config\.[cm]?[jt]s$/, /(^|\/)tsconfig[^/]*\.json$/, + /^\.github\//, /^\.gitlab-ci\.yml$/, /(^|\/)Makefile$/, + /(^|\/)(tests?|__tests__|spec)\//, /\.(test|spec)\.[cm]?[jt]sx?$/, /_test\.(go|py|rs)$/, +]; + +export function protectedPathRefusal(paths: readonly string[]): string | undefined { + const hit = paths.find((path) => PROTECTED_PATHS.some((pattern) => pattern.test(path))); + return hit === undefined ? undefined : `it touched ${hit}, which the reviewer may not change (tests, their runner configuration, lockfiles and CI are human-owned)`; } function reviewContext(meta: PrMeta, pr: Pr): Record { diff --git a/examples/pr-reviewer/tests/pr-state.test.ts b/examples/pr-reviewer/tests/pr-state.test.ts index 26e6f6230..b955f7220 100644 --- a/examples/pr-reviewer/tests/pr-state.test.ts +++ b/examples/pr-reviewer/tests/pr-state.test.ts @@ -22,6 +22,11 @@ import { prReadyStateAllowsHumanReview, prReviewStateFromRest, prFromInput, + mergeRefusal, + approvedCommit, + protectedPathRefusal, + testCommand, + reviewer, readPr, resolveAuthorLogin, reviewHarnessPrompt, @@ -30,6 +35,7 @@ import { shouldSkipReview, shellWord, } from "../pr-reviewer.flow.ts"; +import { getFlowDefinition } from "@relayflows/surface/runtime"; test('reviewAuthorAllowlistDecision lets configured authors through', () => { assert.equal(reviewAuthorAllowlistDecision(new Set(['willwashburn']), 'willwashburn'), null); @@ -607,3 +613,59 @@ test("HARNESS_RESOURCE_ENV holds the test run to one worker under the sandbox he const heapMib = Number(/--max-old-space-size=(\d+)/.exec(HARNESS_RESOURCE_ENV.NODE_OPTIONS ?? "")?.[1]); assert.ok(heapMib <= 8192 - 2048); }); + +test("prFromInput refuses an event that names a different PR than the configured one", () => { + const event = { pull_request: { number: 9, user: { login: "opener" } }, repository: { name: "other", owner: { login: "o" } } }; + assert.throws(() => prFromInput({ owner: "o", repo: "r", number: 9, approvers: "", event }), /names o\/other#9, not the configured o\/r#9/); + assert.throws(() => prFromInput({ owner: "o", repo: "r", number: 3, approvers: "", event: { ...event, repository: { name: "r", owner: { login: "o" } } } }), /#9, not the configured o\/r#3/); + // Agreeing coordinates enrich the PR with the payload's author. + assert.equal(prFromInput({ owner: "O", repo: "R", number: 9, approvers: "", event: { ...event, repository: { name: "r", owner: { login: "o" } } } }).author, "opener"); +}); + +test("mergeRefusal only lets an approval merge the approved, green, mergeable head", () => { + const pr = { owner: "o", repo: "r", number: 1, url: "u", author: "a" }; + const green = prReviewStateFromRest({ state: "open", mergeable: true, mergeable_state: "clean", head: { sha: "abc" } }, + { check_runs: [{ name: "ci", status: "completed", conclusion: "success" }] }, []); + assert.equal(mergeRefusal(green, pr, "abc"), undefined); + assert.equal(pr.headSha, "abc"); + assert.match(mergeRefusal(green, pr, "def") ?? "", /approval was for def, but the head is now abc/); + assert.match(mergeRefusal({ ...green, headRefOid: undefined }, pr, undefined) ?? "", /head SHA could not be read/); + const red = prReviewStateFromRest({ state: "open", mergeable: true, mergeable_state: "clean", head: { sha: "abc" } }, + { check_runs: [{ name: "ci", status: "completed", conclusion: "failure" }] }, []); + assert.match(mergeRefusal(red, pr, "abc") ?? "", /check=ci/); + // An approval without a commit id merges only when the head is green (no stale check possible). + assert.equal(mergeRefusal(green, pr, undefined), undefined); + assert.equal(approvedCommit({ review: { commit_id: "0123abc" } }), "0123abc"); + assert.equal(approvedCommit({ review: { commit_id: "not a sha" } }), undefined); +}); + +test("commit statuses count toward READY alongside check runs", () => { + const meta = { state: "open", mergeable: true, mergeable_state: "clean", head: { sha: "abc" } }; + const checks = { check_runs: [{ name: "unit", status: "completed", conclusion: "success" }] }; + assert.equal(prReadyStateAllowsHumanReview(prReviewStateFromRest(meta, checks, [], { statuses: [] })), true); + const pendingPreview = { statuses: [{ context: "deploy-preview", state: "pending" }] }; + assert.equal(prReadyStateAllowsHumanReview(prReviewStateFromRest(meta, checks, [], pendingPreview)), false); + const failedPreview = { statuses: [{ context: "deploy-preview", state: "failure" }] }; + assert.equal(prReadyStateAllowsHumanReview(prReviewStateFromRest(meta, checks, [], failedPreview)), false); + assert.equal(prReadyStateAllowsHumanReview(prReviewStateFromRest(meta, checks, [], { statuses: [{ context: "x", state: "success" }] })), true); +}); + +test("protectedPathRefusal names the first human-owned path and lets ordinary sources through", () => { + assert.equal(protectedPathRefusal(["src/a.ts", "README.md"]), undefined); + for (const path of ["package.json", "pkg/package.json", "package-lock.json", "yarn.lock", ".github/workflows/ci.yml", + "tests/a.test.ts", "src/__tests__/b.ts", "src/c.spec.tsx", "vitest.config.ts", "tsconfig.json", "Cargo.toml", "foo_test.go", "Makefile"]) { + assert.match(protectedPathRefusal(["src/ok.ts", path]) ?? "", new RegExp(`touched ${path.replace(/[.*+?^${}()|[\\]\\]/g, "\\$&")}`), path); + } +}); + +test("testCommand is pinned from input, defaults to npm test, and must be one line", () => { + assert.equal(testCommand({}), "npm test"); + assert.equal(testCommand({ testCommand: " cargo test --locked " }), "cargo test --locked"); + assert.throws(() => testCommand({ testCommand: "npm test\nrm -rf /" }), /single shell line/); +}); + +test("the exported handle carries the three PR trigger handlers", () => { + const definition = getFlowDefinition(reviewer); + assert.equal(definition.name, "pr-reviewer"); + assert.equal(definition.handlers.length, 3); +}); From afbf2ba8be1959ddda6ed816e04d28ed5296383f Mon Sep 17 00:00:00 2001 From: Relayflow Lead Date: Thu, 17 Sep 2026 15:30:57 -0700 Subject: [PATCH 3/3] examples(pr-reviewer): never READY on a just-pushed head; conflict commands fail closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - After the flow pushes mechanical fixes, the PR head is a commit whose CI has not run, so READY is withheld for that pass (the comment says the PR is re-checked on the next synchronize) instead of being judged on the superseded head's checks — which is what the fixture's constant head SHA had been letting through. Proven three ways through the real kernel: edit+push → no ready line; no edit + green → ready; no edit + a check `in_progress` → no ready line. - `isAuthorizedConflictCommander` fails closed to the PR author when no trust lists are configured (v4 was open to any non-bot commenter). Co-Authored-By: Claude Opus 5 (1M context) --- examples/pr-reviewer/README.md | 9 +++++++- .../evidence/api-calls-pending.txt | 12 ++++++++++ .../pr-reviewer/evidence/api-calls-ready.txt | 14 ++++++++++++ .../evidence/api-calls-success.txt | 6 +---- examples/pr-reviewer/evidence/curl-shim.sh | 2 +- .../evidence/origin-feature-after.txt | 2 +- examples/pr-reviewer/pr-reviewer.flow.ts | 22 ++++++++++--------- examples/pr-reviewer/tests/pr-state.test.ts | 5 +++-- 8 files changed, 52 insertions(+), 20 deletions(-) create mode 100644 examples/pr-reviewer/evidence/api-calls-pending.txt create mode 100644 examples/pr-reviewer/evidence/api-calls-ready.txt diff --git a/examples/pr-reviewer/README.md b/examples/pr-reviewer/README.md index 39f015d75..b0813d8bc 100644 --- a/examples/pr-reviewer/README.md +++ b/examples/pr-reviewer/README.md @@ -83,6 +83,8 @@ inputs and the tests themselves, which are human-owned); and the PR head lives in this repository (a fork's head is another repo, so fixes for fork PRs are posted as advisory, not pushed). A protected-path edit also vetoes READY, because a green run that came with a rewritten test script proves nothing. +So does a push: the new head's CI has not run yet, so the comment says the PR +will be re-checked and the next `synchronize` pass judges the new head. Beyond that, "mechanical only" is the prompt's contract, as it was in v4 — the review lists every changed path, and the push is a normal commit a human can revert. @@ -92,6 +94,9 @@ from an allowlisted approver (empty `approvers` = anyone, the upstream default — set it), the approval's `commit_id` equal to the current head (or no `commit_id` at all), the live state green and mergeable, and the merge call carries that head SHA so GitHub refuses if the head moves in between. +A conflict-resolution command (`@relay fix conflicts`, not yet wired) is +taken only from the PR author or a listed approver/reviewer — with no lists +configured it fails closed to the author, unlike v4's open default. A webhook payload may enrich the configured coordinates (author, head, labels) but never redirect them: an event naming a different PR is refused. @@ -134,7 +139,9 @@ bare scratch `origin` with `refs/pull/7/head`. | Path | Result | | --- | --- | -| happy: tests green | 18 steps `success`; review commit pushed to the PR branch ([origin-feature-after.txt](evidence/origin-feature-after.txt)); comment posted with the READY sentinel stripped and the `:white_check_mark:` line present; check runs **and** commit statuses read ([api-calls-success.txt](evidence/api-calls-success.txt)) | +| happy: tests green, agent made a mechanical edit | 14 steps `success`; review commit pushed to the PR branch ([origin-feature-after.txt](evidence/origin-feature-after.txt)); comment posted with the sentinel stripped and **no** ready line — a push means the new head is unverified — plus the re-check note ([api-calls-success.txt](evidence/api-calls-success.txt)) | +| no edit, checks green | 17 steps `success`; nothing pushed; check runs **and** commit statuses read for the head; `:white_check_mark:` line present ([api-calls-ready.txt](evidence/api-calls-ready.txt)) | +| no edit, a check `in_progress` | 17 steps `success`; comment posted, **no** ready line ([api-calls-pending.txt](evidence/api-calls-pending.txt)) | | red: `npm test` exits 1 | 15 steps `success`; PR branch untouched; edits discarded; advisory posted; **no** ready line ([api-calls-red.txt](evidence/api-calls-red.txt)) | | fork PR (`head.repo` ≠ base) | 19 steps `success`; base repo's branch untouched; advisory names the fork ([api-calls-fork.txt](evidence/api-calls-fork.txt)) | | protected path: the agent rewrites `package.json`'s test script so tests "pass" | 15 steps `success`; nothing pushed; advisory names `package.json`; **no** ready line despite the green run ([api-calls-protected.txt](evidence/api-calls-protected.txt)) | diff --git a/examples/pr-reviewer/evidence/api-calls-pending.txt b/examples/pr-reviewer/evidence/api-calls-pending.txt new file mode 100644 index 000000000..4a014fb1c --- /dev/null +++ b/examples/pr-reviewer/evidence/api-calls-pending.txt @@ -0,0 +1,12 @@ +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/comments?per_page=100 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/reviews?per_page=100 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/commits/6dee8f6fa764906cf2c7039ff217a991a6eb315b/check-runs?per_page=100 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/commits/6dee8f6fa764906cf2c7039ff217a991a6eb315b/status +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/reviews?per_page=100 +-sf -X POST -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/issues/7/comments -d {"body":"## Review +Looks fine. + +## Addressed comments +- none"} diff --git a/examples/pr-reviewer/evidence/api-calls-ready.txt b/examples/pr-reviewer/evidence/api-calls-ready.txt new file mode 100644 index 000000000..a71bc70a0 --- /dev/null +++ b/examples/pr-reviewer/evidence/api-calls-ready.txt @@ -0,0 +1,14 @@ +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/comments?per_page=100 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/reviews?per_page=100 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/commits/6dee8f6fa764906cf2c7039ff217a991a6eb315b/check-runs?per_page=100 +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/commits/6dee8f6fa764906cf2c7039ff217a991a6eb315b/status +-sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/reviews?per_page=100 +-sf -X POST -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/issues/7/comments -d {"body":"## Review +Looks fine. + +## Addressed comments +- none + +:white_check_mark: This PR is ready for your review."} diff --git a/examples/pr-reviewer/evidence/api-calls-success.txt b/examples/pr-reviewer/evidence/api-calls-success.txt index a71bc70a0..48c26adc7 100644 --- a/examples/pr-reviewer/evidence/api-calls-success.txt +++ b/examples/pr-reviewer/evidence/api-calls-success.txt @@ -1,14 +1,10 @@ -sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7 -sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/comments?per_page=100 -sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/reviews?per_page=100 --sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7 --sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/commits/6dee8f6fa764906cf2c7039ff217a991a6eb315b/check-runs?per_page=100 --sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/commits/6dee8f6fa764906cf2c7039ff217a991a6eb315b/status --sf -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/pulls/7/reviews?per_page=100 -sf -X POST -H Authorization: Bearer -H Accept: application/vnd.github+json https://api.github.com/repos/o/r/issues/7/comments -d {"body":"## Review Looks fine. ## Addressed comments - none -:white_check_mark: This PR is ready for your review."} +Mechanical fixes were pushed; this PR will be re-checked once CI runs on the new head."} diff --git a/examples/pr-reviewer/evidence/curl-shim.sh b/examples/pr-reviewer/evidence/curl-shim.sh index f310e1b6b..ad2b3264c 100755 --- a/examples/pr-reviewer/evidence/curl-shim.sh +++ b/examples/pr-reviewer/evidence/curl-shim.sh @@ -8,7 +8,7 @@ case "$url" in */pulls/[0-9]*/comments*) printf '[]' ;; */pulls/[0-9]*/reviews*) printf '[]' ;; */pulls/[0-9]*) printf '{"state":"open","draft":%s,"merged":false,"mergeable":true,"mergeable_state":"clean","user":{"login":"octocat"},"labels":[],"head":{"sha":"%s","ref":"%s","repo":{"full_name":"%s"}},"base":{"ref":"main"},"html_url":"https://github.com/o/r/pull/7","title":"feature"}' "$draft" "$PRPROOF_HEAD" "$PRPROOF_REF" "${PRPROOF_HEAD_REPO:-o/r}" ;; - */commits/*/check-runs*) printf '{"check_runs":[{"name":"ci","status":"completed","conclusion":"success"}]}' ;; + */commits/*/check-runs*) printf '{"check_runs":[{"name":"ci","status":"%s","conclusion":%s}]}' "${PRPROOF_CHECK_STATUS:-completed}" "$( [ "${PRPROOF_CHECK_STATUS:-completed}" = completed ] && echo '"success"' || echo null )" ;; */commits/*/status) printf '{"state":"success","statuses":[]}' ;; */issues/[0-9]*/comments) printf '{"id":1}' ;; *) echo "unexpected $url" >&2; exit 22 ;; diff --git a/examples/pr-reviewer/evidence/origin-feature-after.txt b/examples/pr-reviewer/evidence/origin-feature-after.txt index 6f9ff32f1..4b4040a3b 100644 --- a/examples/pr-reviewer/evidence/origin-feature-after.txt +++ b/examples/pr-reviewer/evidence/origin-feature-after.txt @@ -1,3 +1,3 @@ -2184542 review: mechanical fixes +dac0381 review: mechanical fixes 6dee8f6 feature fd311b2 base diff --git a/examples/pr-reviewer/pr-reviewer.flow.ts b/examples/pr-reviewer/pr-reviewer.flow.ts index 54faf92d9..ae460a045 100644 --- a/examples/pr-reviewer/pr-reviewer.flow.ts +++ b/examples/pr-reviewer/pr-reviewer.flow.ts @@ -162,9 +162,11 @@ const reviewerBody = flow( : verified.trim() !== "PASS" ? "the repository's test command was red in the review sandbox (see .workforce/test.log)" : !sameRepo ? `the PR head lives in ${headRepo}, not in ${pr.owner}/${pr.repo}, and this flow only pushes to its own repository` : undefined; + let pushed = false; if (changedPaths.length > 0 && pushRefusal === undefined) { // Mechanical fixes, verified by the pinned test command, go to the PR. await f.run(`git -c user.name=Relayflow -c user.email=noreply@agentrelay.com commit -q -m "review: mechanical fixes" && git push origin ${shellWord(`HEAD:refs/heads/${headRef}`)}`, { timeout: "5m" }); + pushed = true; } else if (changedPaths.length > 0) { // An unverified or out-of-bounds push is worse than no push: keep the // diff for the review, discard the edits, and say so. @@ -175,9 +177,11 @@ const reviewerBody = flow( // Only call it a human's turn when it actually is: the agent said READY, // the tests this flow ran were green (a signal the v4 agent never had), - // and GitHub's live state agrees. - const ready = harnessReady && trustedGreen && prReadyStateAllowsHumanReview(await readPrReviewState(api, pr)); - const body = ready ? `${review}\n\n:white_check_mark: This PR is ready for your review.` : review; + // nothing was just pushed (a new head's CI has not run — the next + // `synchronize` pass judges it), and GitHub's live state agrees. + const ready = harnessReady && trustedGreen && !pushed && prReadyStateAllowsHumanReview(await readPrReviewState(api, pr)); + const body = ready ? `${review}\n\n:white_check_mark: This PR is ready for your review.` + : pushed ? `${review}\n\nMechanical fixes were pushed; this PR will be re-checked once CI runs on the new head.` : review; await postComment(f, input, pr, body); f.done("success"); }, @@ -929,9 +933,10 @@ export function matchesConflictDirective(body: string): boolean { } /** - * Who may command a conflict resolution. A bot never qualifies, and when - * approvers/reviewAuthors are configured the commenter must be the PR's own - * author or appear on one of those lists. With neither list set, anyone. + * Who may command a conflict resolution — it force-updates the PR branch, so + * the order is taken only from the PR's own author or someone on a trust + * list. A bot never qualifies. Unlike v4 (open to everyone with no lists + * set), an empty configuration fails closed to the author alone. */ export function isAuthorizedConflictCommander( config: { approvers?: string | undefined; reviewAuthors?: string | undefined }, @@ -939,11 +944,8 @@ export function isAuthorizedConflictCommander( pr: Pr, ): boolean { if (!commander || commander.endsWith("[bot]")) return false; - const approvers = [...commaSet(config.approvers)]; - const reviewAuthors = commaSet(config.reviewAuthors); - if (approvers.length === 0 && reviewAuthors.size === 0) return true; if (commander === (pr.author ?? "").trim().toLowerCase()) return true; - return approvers.includes(commander) || reviewAuthors.has(commander); + return commaSet(config.approvers).has(commander) || commaSet(config.reviewAuthors).has(commander); } export function isApproval(payload: unknown): boolean { diff --git a/examples/pr-reviewer/tests/pr-state.test.ts b/examples/pr-reviewer/tests/pr-state.test.ts index b955f7220..89719c98d 100644 --- a/examples/pr-reviewer/tests/pr-state.test.ts +++ b/examples/pr-reviewer/tests/pr-state.test.ts @@ -199,9 +199,10 @@ test('isAuthorizedConflictCommander never takes the order from a bot', () => { assert.equal(isAuthorizedConflictCommander({}, '', pr), false); }); -test('isAuthorizedConflictCommander is open when no trust lists are configured', () => { +test('isAuthorizedConflictCommander fails closed to the PR author when no trust lists are configured', () => { const pr = { owner: 'AgentWorkforce', repo: 'agents', number: 1, author: 'willwashburn' }; - assert.equal(isAuthorizedConflictCommander({}, 'anyone', pr), true); + assert.equal(isAuthorizedConflictCommander({}, 'anyone', pr), false); + assert.equal(isAuthorizedConflictCommander({}, 'willwashburn', pr), true); }); test('isAuthorizedConflictCommander gates on APPROVERS/REVIEW_AUTHORS and the PR author', () => {