diff --git a/.github/scripts/agent-audience.mjs b/.github/scripts/agent-audience.mjs new file mode 100644 index 00000000..4b76d3d5 --- /dev/null +++ b/.github/scripts/agent-audience.mjs @@ -0,0 +1,136 @@ +// agent-audience (#911): a pull request names its owner and its reviewer in the body, and the review +// round calls that reviewer. When a round calls somebody else, the named reviewer is never asked and +// the pull request sits with no verdict while everyone believes review is in flight — #907 did that +// for four tips. This is a report, not a gate: it reads the two body lines and the comments written +// after the current tip, and says where they disagree. Silence is not detectable here (the repository +// has no "who read this" field); a comment that calls nobody is invisible to this check. +// +// node .github/scripts/agent-audience.mjs --repo ThinkFlowLab/FlyBot --pr 907 +// node .github/scripts/agent-audience.mjs --repo ThinkFlowLab/FlyBot # every open PR +// +// Report only: the process exits 0 whatever it finds. Every line has the shape +// `# @: ` so a round can read it without opening the pull request. +import { execFileSync } from 'node:child_process' +import { readFileSync } from 'node:fs' +import { fileURLToPath } from 'node:url' + +export const POLICY_PATH = new URL('../agent/review-policy.json', import.meta.url) +// The Chief escalates a stalled review; its call is not a wrong call. +export const ALWAYS_ALLOWED = ['Chief'] + +const escape = value => String(value).replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + +export function loadPolicy(path = POLICY_PATH) { + return JSON.parse(readFileSync(path, 'utf8')) +} + +// The roster the review policy already carries. A declared name outside it is a typo or a member who +// cannot write a verdict at all, and either way the round calls nobody who can answer. +export function rosterOf(policy = {}) { + const names = new Set() + for (const key of ['writers', 'reviewers', 'maintainers']) { + for (const name of policy[key] ?? []) if (typeof name === 'string' && name) names.add(name) + } + for (const name of policy.selfReview ?? []) if (typeof name === 'string' && name) names.add(name) + return names +} + +// `Owner: Dev2` / `Reviewer: Reviewer2` on their own line. Deliberately not a search anywhere in the +// body: quoting the requirement in prose must not satisfy it. +export function parseDeclarations(body = '') { + const line = name => { + const match = new RegExp(`^${name}:\\s*(\\S+)\\s*$`, 'm').exec(String(body ?? '')) + return match ? match[1] : undefined + } + return { owner: line('Owner'), reviewer: line('Reviewer') } +} + +// The policy names a member's `@` handle exactly, so a mention is matched whole: `@Reviewer` must not +// match inside `@Reviewer2`, and a handle longer than a roster name is not that member. +export function mentionsIn(text = '', names) { + const found = [] + for (const name of names) { + const pattern = new RegExp(`(^|[^A-Za-z0-9_.-])@${escape(name)}(?![A-Za-z0-9_.-])`, 'g') + if (pattern.test(String(text ?? ''))) found.push(name) + } + return found.sort() +} + +const sha7 = sha => String(sha ?? '').slice(0, 7) +const report = (pr, sentence) => `#${pr.number} @${sha7(pr.headSha)}: ${sentence}` + +// One report per defect, not per comment: three calls to the wrong member are one disagreement. +export function evaluate({ pr, comments = [], policy = {}, tipCommittedAt, allow = ALWAYS_ALLOWED } = {}) { + const { owner, reviewer } = parseDeclarations(pr.body) + const roster = rosterOf(policy) + const out = [] + const missing = [] + if (!owner) missing.push('`Owner:`') + if (!reviewer) missing.push('`Reviewer:`') + if (missing.length) out.push(report(pr, `pull request body has no ${missing.join(' and ')} line`)) + for (const [label, name] of [['Owner', owner], ['Reviewer', reviewer]]) { + if (!name || roster.has(name)) continue + out.push(report(pr, `${label}: names \`${name}\`, which is not in the review-policy roster`)) + } + // Only calls written after the tip can be about this review round; an earlier call belongs to the + // tip it was written for, and its tip is already void. + const since = tipCommittedAt ? Date.parse(tipCommittedAt) : 0 + const calls = new Map() + for (const comment of comments) { + const at = Date.parse(comment.createdAt ?? '') + if (!Number.isFinite(at) || (Number.isFinite(since) && since > 0 && at <= since)) continue + for (const name of mentionsIn(comment.body, roster)) { + if (name === reviewer || allow.includes(name)) continue + const entry = calls.get(name) ?? { count: 0, ids: [] } + entry.count += 1 + if (comment.id !== undefined) entry.ids.push(comment.id) + calls.set(name, entry) + } + } + for (const [name, call] of [...calls].sort(([a], [b]) => a.localeCompare(b))) { + const where = call.ids.length ? ` (comment ${call.ids.join(', ')})` : '' + const named = reviewer ? `the body names \`Reviewer: ${reviewer}\`` : 'the body names no `Reviewer:`' + out.push(report(pr, `${call.count} call${call.count === 1 ? '' : 's'} to @${name} after this tip${where}, but ${named}`)) + } + return out +} + +const realGh = (...args) => execFileSync('gh', args, { encoding: 'utf8', maxBuffer: 16 << 20 }) + +// `gh` and `env` are injectable so the run can be driven from fixtures, the way label-hygiene.mjs is. +export function main({ gh = realGh, env = process.env, repo: repoOption, pr: prOption, policy = loadPolicy() } = {}) { + const arg = (name) => { const i = process.argv.indexOf(name); return i > 0 ? process.argv[i + 1] : undefined } + const repo = repoOption || env.GH_REPO || arg('--repo') + if (!repo) throw new Error('set GH_REPO or pass --repo owner/name') + const single = prOption ?? arg('--pr') + const page = (path) => gh('api', '--paginate', '--jq', '.[]', path).split('\n').filter(Boolean).map(line => JSON.parse(line)) + + const numbers = single + ? [Number(single)] + : page(`repos/${repo}/pulls?state=open&per_page=100`).map(pull => pull.number) + const reports = [] + for (const number of numbers) { + const pull = JSON.parse(gh('api', `repos/${repo}/pulls/${number}`)) + const comments = page(`repos/${repo}/issues/${number}/comments?per_page=100`).map(c => ({ id: c.id, body: c.body, createdAt: c.created_at })) + const tipCommittedAt = gh('api', '--jq', '.commit.committer.date', `repos/${repo}/commits/${pull.head.sha}`).trim() + reports.push(...evaluate({ + pr: { number, body: pull.body, headSha: pull.head.sha }, + comments, + policy, + tipCommittedAt, + })) + } + for (const line of reports) console.log(line) + console.log(`agent-audience: ${reports.length} finding(s) on ${numbers.length} pull request(s); report only`) + const summary = env.GITHUB_STEP_SUMMARY + if (summary) { + const body = reports.length ? `${reports.join('\n')}\n` : 'agent-audience: no findings\n' + // Best effort: a missing summary must not change the check's result. + try { execFileSync('sh', ['-c', `cat >> "$1"`, 'sh', summary], { input: `## agent-audience\n\n${body}`, encoding: 'utf8' }) } catch {} + } + return { ok: true, reports, numbers } +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + try { main() } catch (error) { console.error(`agent-audience: ${error.message}`); process.exit(1) } +} diff --git a/.github/scripts/agent-audience.test.mjs b/.github/scripts/agent-audience.test.mjs new file mode 100644 index 00000000..1ffaec1a --- /dev/null +++ b/.github/scripts/agent-audience.test.mjs @@ -0,0 +1,131 @@ +// agent-audience (#911): the check reads the two lines a pull request body must carry and the calls +// written after its current tip, and reports where they disagree. The first case replays the real +// #907 record: the body of that pull request carried neither line, and the review rounds called +// `@Reviewer` three times while the declared reviewer (in the team chat) was Reviewer2. Those calls +// were written in the delivery group rather than on the pull request, which is why the replay feeds +// them as comment bodies; the live run sees only the GitHub comments, and reports the missing lines. +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { readFileSync } from 'node:fs' +import { evaluate, loadPolicy, main, mentionsIn, parseDeclarations, rosterOf } from './agent-audience.mjs' + +const policy = loadPolicy() +const TIP = '0c7b33a7b817eddc2c15c7edaeb20eca9f938c85' +const TIP_AT = '2026-10-24T01:27:48Z' + +// The first lines of the real #907 body: no `Owner:`/`Reviewer:` anywhere. +const BODY_907 = `Written-by: Dev3 + +Fixes #904 + +A guest's copy of a linked room now lists the host's Bots and shows the room's group notice.` + +// The three rounds that called `@Reviewer` for #907, at the times the group recorded them. +const callsTo907 = [ + { id: 9101, createdAt: '2026-10-24T01:27:56Z', body: '@Reviewer 请按 `0c7b33a7` 判' }, + { id: 9102, createdAt: '2026-10-24T01:28:30Z', body: '@Reviewer 这张归你判，不是 Reviewer2' }, + { id: 9103, createdAt: '2026-10-24T01:29:10Z', body: '@Reviewer 等结论' }, +] + +const pr907 = { number: 907, body: BODY_907, headSha: TIP } + +test('#911: the #907 record is two findings — no declared reviewer, and three calls to @Reviewer', () => { + const reports = evaluate({ pr: pr907, comments: callsTo907, policy, tipCommittedAt: TIP_AT }) + assert.deepEqual(reports, [ + '#907 @0c7b33a: pull request body has no `Owner:` and `Reviewer:` line', + '#907 @0c7b33a: 3 calls to @Reviewer after this tip (comment 9101, 9102, 9103), but the body names no `Reviewer:`', + ]) +}) + +test('#911: negative control — the declared reviewer being called produces no finding', () => { + const body = `Owner: Dev3\nReviewer: Reviewer2\n\nFixes #904\nTip SHA: ${TIP}` + const comments = [ + { id: 9201, createdAt: '2026-10-24T01:28:00Z', body: '@Reviewer2 please judge `0c7b33a7`' }, + { id: 9202, createdAt: '2026-10-24T01:28:40Z', body: 'Written-by: Reviewer2 — running the suite now' }, + ] + assert.deepEqual(evaluate({ pr: { number: 904, body, headSha: TIP }, comments, policy, tipCommittedAt: TIP_AT }), []) +}) + +test('#911: a call written before the current tip belongs to the tip it was written for', () => { + const body = `Owner: Dev3\nReviewer: Reviewer2` + const before = [{ id: 9301, createdAt: '2026-10-24T00:10:00Z', body: '@Reviewer old round' }] + assert.deepEqual(evaluate({ pr: { number: 5, body, headSha: TIP }, comments: before, policy, tipCommittedAt: TIP_AT }), []) +}) + +test('#911: Chief is the escalation exit, not a wrong call', () => { + const body = `Owner: Dev3\nReviewer: Reviewer2` + const comments = [{ id: 9401, createdAt: '2026-10-24T01:30:00Z', body: '@Chief this PR has waited four rounds' }] + assert.deepEqual(evaluate({ pr: { number: 5, body, headSha: TIP }, comments, policy, tipCommittedAt: TIP_AT }), []) +}) + +test('#911: a name outside the review policy is its own finding', () => { + const body = `Owner: Dev3\nReviewer: Reviewer3` + const reports = evaluate({ pr: { number: 5, body, headSha: TIP }, comments: [], policy, tipCommittedAt: TIP_AT }) + assert.deepEqual(reports, ['#5 @0c7b33a: Reviewer: names `Reviewer3`, which is not in the review-policy roster']) +}) + +test('#911: @Reviewer2 is not read as @Reviewer, and the body lines are matched whole', () => { + assert.deepEqual(mentionsIn('@Reviewer2 please judge', new Set(['Reviewer', 'Reviewer2'])), ['Reviewer2']) + assert.deepEqual(mentionsIn('cite @Reviewer.example in text', new Set(['Reviewer'])), []) + assert.deepEqual(parseDeclarations('Owner: Dev2\nReviewer: Reviewer2'), { owner: 'Dev2', reviewer: 'Reviewer2' }) + // Prose that quotes the requirement does not satisfy it. + assert.deepEqual(parseDeclarations('Write `Owner: Dev2` and `Reviewer: Reviewer2` in the body'), { owner: undefined, reviewer: undefined }) + assert.deepEqual(parseDeclarations('Owner: Dev2 and Reviewer: Reviewer2 on one line'), { owner: undefined, reviewer: undefined }) +}) + +test('#911: the roster is the policy file, not a second list', () => { + const roster = rosterOf(policy) + for (const name of ['Chief', 'Dev2', 'Reviewer2', 'Birfy']) assert.ok(roster.has(name), name) + assert.equal(roster.has('Reviewer3'), false) +}) + +// A fake `gh` over recorded API answers, the way label-hygiene.mjs is tested. +const fakeGh = (answers) => { + const calls = [] + const gh = (...args) => { + calls.push(args.join(' ')) + const path = args[args.length - 1] + const key = Object.keys(answers).find(prefix => path.startsWith(prefix)) + if (!key) throw new Error(`unexpected gh call: ${args.join(' ')}`) + const value = answers[key] + return typeof value === 'function' ? value(args) : value + } + gh.calls = calls + return gh +} + +test('#911: the run prints one line per finding and exits success by design', () => { + const gh = fakeGh({ + 'repos/x/y/pulls/907': JSON.stringify({ number: 907, body: BODY_907, head: { sha: TIP } }), + 'repos/x/y/issues/907/comments': callsTo907.map(c => JSON.stringify({ id: c.id, body: c.body, created_at: c.createdAt })).join('\n'), + 'repos/x/y/commits/': `${TIP_AT}\n`, + }) + const lines = [] + const original = console.log + console.log = (...args) => lines.push(args.join(' ')) + try { + const result = main({ gh, env: {}, repo: 'x/y', pr: '907', policy }) + assert.equal(result.ok, true) + assert.equal(result.reports.length, 2) + } finally { + console.log = original + } + assert.match(lines[0], /^#907 @0c7b33a: /) + assert.match(lines.at(-1), /2 finding\(s\) on 1 pull request\(s\); report only/) +}) + +// #912: this check passed on its own pull request while reading nothing. The workflow checked the +// script out of `main`, where it does not exist yet, `node` could not load it, and `continue-on-error` +// turned that crash into a green check -- a check that passes without reading is the failure this job +// exists to report. It now reads the script from the base commit (a pull request must not swap the +// checker that audits it) and falls back to the pull request for the run that introduces it. Either +// half disappearing turns this red, so the guard names both. +test('#911: the workflow cannot pass without having read the checker', () => { + const yml = readFileSync(new URL('../workflows/agent-audience.yml', import.meta.url), 'utf8') + assert.match(yml, /ref: \$\{\{ github\.event\.pull_request\.base\.sha \|\| github\.event\.repository\.default_branch \}\}/) + assert.match(yml, /if: hashFiles\('base\/\.github\/scripts\/agent-audience\.mjs'\) == ''/) + assert.match(yml, /dir="pr"/) + assert.match(yml, /node "\$dir\/\.github\/scripts\/agent-audience\.mjs"/) + assert.doesNotMatch(yml, /node \.github\/scripts\/agent-audience\.mjs/) + assert.doesNotMatch(yml, /^\s+ref: main$/m) +}) diff --git a/.github/workflows/agent-audience.yml b/.github/workflows/agent-audience.yml new file mode 100644 index 00000000..f3a9e7e4 --- /dev/null +++ b/.github/workflows/agent-audience.yml @@ -0,0 +1,77 @@ +name: Agent audience + +# A pull request names its owner and its reviewer in the body, and the review round calls that +# reviewer. A round that calls somebody else — or a body that names nobody — leaves the pull request +# waiting for a verdict while it looks like review is in flight (#911: #907 sat that way for four +# tips, and every call went to a member who was not the declared reviewer). +# +# This is a report, not a gate: `continue-on-error` keeps the check green whatever it finds, it only +# reads, and it writes the findings into the job summary. It never comments and never labels. +# Silence itself is not detectable (the repository has no "who read this" field); only a call to the +# wrong member, or a missing declaration, is. +# +# The one way this check can lie is by passing without reading anything: `continue-on-error` turns a +# crash into a green check, so the script must always be found. It is therefore checked out from the +# BASE commit -- a pull request must not be able to change the checker that audits it -- with the +# first-run fallback review-gate.yml uses: the pull request that introduces the script has no copy on +# the base yet, so that one run reads the copy in the pull request, under the same read-only token. +on: + pull_request: + types: [opened, edited, synchronize, reopened, ready_for_review] + issue_comment: + types: [created] + workflow_dispatch: + +concurrency: + group: agent-audience-${{ github.event.pull_request.number || github.event.issue.number || 'all' }} + cancel-in-progress: true + +permissions: + contents: read + issues: read + pull-requests: read + +jobs: + audience: + name: agent-audience + # A comment on a plain issue is not a pull request. + if: github.event_name != 'issue_comment' || github.event.issue.pull_request != null + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ github.event.pull_request.base.sha || github.event.repository.default_branch }} + path: base + persist-credentials: false + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + if: hashFiles('base/.github/scripts/agent-audience.mjs') == '' + with: + ref: ${{ github.event.pull_request.head.sha || github.event.repository.default_branch }} + path: pr + persist-credentials: false + - uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 + with: + node-version: 24 + - name: Read the declared audience against the calls written this round + continue-on-error: true + env: + GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} + PR: ${{ github.event.pull_request.number || github.event.issue.number }} + run: | + dir="base" + if [ ! -f base/.github/scripts/agent-audience.mjs ]; then + echo "::notice::The base commit has no agent-audience script yet; using the copy in this pull request." + dir="pr" + fi + if [ ! -f "$dir/.github/scripts/agent-audience.mjs" ]; then + # Loud on purpose: a green check that read nothing is the failure this job exists to catch. + echo "::error::agent-audience: the checker is on neither the base commit nor this pull request; nothing was read." + exit 1 + fi + if [ -n "$PR" ]; then + node "$dir/.github/scripts/agent-audience.mjs" --pr "$PR" + else + node "$dir/.github/scripts/agent-audience.mjs" + fi diff --git a/docs/testing-layers.json b/docs/testing-layers.json index 8ec026ba..767a326f 100644 --- a/docs/testing-layers.json +++ b/docs/testing-layers.json @@ -228,6 +228,15 @@ "gate": false, "runs": "pull_request_review (submitted)", "reason": "Says, in one warning and the run summary, that a PASS posted as a review body whose Written-by names someone outside the policy's writers does not count (#801). Read-only by construction (permissions: contents: read), so a pull request that edits it can only change what its own review job prints; a workflow-only layer with no gate." + }, + { + "id": "agent-audience", + "workflow": ".github/workflows/agent-audience.yml", + "kind": "workflow-only", + "layer": "none", + "gate": false, + "runs": "pull_request (opened, edited, synchronize, reopened, ready_for_review), issue_comment (created) and workflow_dispatch", + "reason": "Reads the body's Owner:/Reviewer: lines and the calls written after the current tip, and prints where they disagree (#911). Report only: continue-on-error, read-only permissions, no comment and no label, so it gates nothing." } ] } diff --git a/docs/testing-layers.md b/docs/testing-layers.md index fd0c98c1..00d76d89 100644 --- a/docs/testing-layers.md +++ b/docs/testing-layers.md @@ -103,5 +103,6 @@ The tables below are generated: edit `docs/testing-layers.json`, then run | `release.yml` | not-a-layer | none | no | — | The mechanical release: preflight, release PR, tag. It runs no tests of its own. | | `pass-label.yml` | workflow-only | none | no | issue_comment (created) and workflow_dispatch | Applies the `pass` label to a pull request whose conclusion was a PASS, and backfills merged ones (#739). A workflow-only layer with no test file of its own, and it gates nothing. | | `review-verdict.yml` | workflow-only | none | no | pull_request_review (submitted) | Says, in one warning and the run summary, that a PASS posted as a review body whose Written-by names someone outside the policy's writers does not count (#801). Read-only by construction (permissions: contents: read), so a pull request that edits it can only change what its own review job prints; a workflow-only layer with no gate. | +| `agent-audience.yml` | workflow-only | none | no | pull_request (opened, edited, synchronize, reopened, ready_for_review), issue_comment (created) and workflow_dispatch | Reads the body's Owner:/Reviewer: lines and the calls written after the current tip, and prints where they disagree (#911). Report only: continue-on-error, read-only permissions, no comment and no label, so it gates nothing. |