diff --git a/.github/issue-owners.json b/.github/issue-owners.json index 726896a63b1..0b0f5a12169 100644 --- a/.github/issue-owners.json +++ b/.github/issue-owners.json @@ -1,5 +1,5 @@ { - "$comment": "Label-driven issue assignment map; areas are keyed on the existing issue label taxonomy. Assignment is a pure function of an issue's labels — no model output is involved. Owners need push access but do NOT need a CODEOWNERS entry; every candidate is re-checked against the collaborator API at write time, so adding a login here cannot grant access to someone who lacks it. Areas match in file order, first match wins. See docs/design/2026-08-07-issue-auto-assignment.md.", + "$comment": "Label-driven issue assignment map; areas are keyed on the existing issue label taxonomy. Assignment is a pure function of an issue's labels — no model output is involved. Owners need push access but do NOT need a CODEOWNERS entry; every candidate is re-checked against the collaborator API at write time, so adding a login here cannot grant access to someone who lacks it. Areas match in file order, first match wins. An area's optional paths list routes PR assignment (assign-pr-owner.mjs) by changed-file prefix, longest prefix wins; the core area's packages/core/ entry is the fallback for paths no module claims. Issue assignment ignores paths; module areas sit after core so label-based issue matching still resolves core first. See docs/design/2026-08-07-issue-auto-assignment.md.", "requireLabels": ["need-discussion"], "skipLabels": [ "welcome-pr", @@ -13,6 +13,7 @@ { "name": "core", "labels": ["category/core", "scope/core"], + "paths": ["packages/core/"], "owners": [ "wenshao", "yiliang114", @@ -30,6 +31,59 @@ "zjunothing", "ZijianZhang989" ] + }, + { + "name": "core-skills", + "labels": ["scope/core"], + "paths": ["packages/core/src/skills/"], + "owners": ["wenshao"] + }, + { + "name": "core-memory", + "labels": ["scope/core"], + "paths": ["packages/core/src/memory/"], + "owners": ["wenshao"] + }, + { + "name": "core-goals", + "labels": ["scope/core"], + "paths": ["packages/core/src/goals/"], + "owners": ["qqqys"] + }, + { + "name": "core-telemetry", + "labels": ["scope/core"], + "paths": ["packages/core/src/telemetry/"], + "owners": ["zjunothing"] + }, + { + "name": "core-extension", + "labels": ["scope/core"], + "paths": ["packages/core/src/extension/"], + "owners": ["callmeYe"] + }, + { + "name": "core-agents", + "labels": ["scope/core"], + "paths": ["packages/core/src/agents/"], + "owners": ["qqqys"] + }, + { + "name": "core-config", + "labels": ["scope/core"], + "paths": ["packages/core/src/config/"], + "owners": ["qqqys"] + }, + { + "name": "core-runtime", + "labels": ["scope/core"], + "paths": [ + "packages/core/src/core/", + "packages/core/src/services/", + "packages/core/src/tools/", + "packages/core/src/utils/" + ], + "owners": ["yiliang114"] } ] } diff --git a/.github/scripts/assign-issue-owner.mjs b/.github/scripts/assign-issue-owner.mjs index 7916aba621d..fbce2e6d493 100644 --- a/.github/scripts/assign-issue-owner.mjs +++ b/.github/scripts/assign-issue-owner.mjs @@ -20,6 +20,19 @@ function isStringArray(value) { return Array.isArray(value) && value.every((v) => typeof v === 'string'); } +// A changed-file prefix for assign-pr-owner.mjs: relative, no `//`, no +// backslash, no `.`/`..` segments, and ending in `/` so startsWith cannot +// leak into a sibling directory (packages/core matching packages/coredump/). +function isPathPrefix(prefix) { + if (typeof prefix !== 'string' || prefix.length === 0) return false; + if (prefix.startsWith('/') || prefix.startsWith('./')) return false; + if (!prefix.endsWith('/')) return false; + if (prefix.includes('\\') || prefix.includes('//')) return false; + return !prefix + .split('/') + .some((segment) => segment === '.' || segment === '..'); +} + export function loadPolicy(raw) { const policy = JSON.parse(raw); if (!policy || typeof policy !== 'object' || Array.isArray(policy)) { @@ -74,6 +87,28 @@ export function loadPolicy(raw) { } seen.add(normalizedOwner); } + // A never-matching paths entry silently unroutes the area from PR + // assignment, so reject it here like other malformed config. + if (area.paths !== undefined && !Array.isArray(area.paths)) { + throw new Error( + `${OWNERS_FILE}: area ${area.name} paths must be an array`, + ); + } + // An explicitly empty list can never route the area either, yet the + // entry loop below cannot catch it — reject it like the sibling + // labels/owners checks do. + if (Array.isArray(area.paths) && area.paths.length === 0) { + throw new Error( + `${OWNERS_FILE}: area ${area.name} paths must not be empty; omit paths for a label-only area`, + ); + } + for (const prefix of area.paths ?? []) { + if (!isPathPrefix(prefix)) { + throw new Error( + `${OWNERS_FILE}: invalid paths entry in ${area.name}: ${JSON.stringify(prefix)}`, + ); + } + } } return policy; } @@ -151,7 +186,7 @@ function canWrite(repository, login) { } } -function openIssueCount(repository, login) { +export function openIssueCount(repository, login) { return Number( gh([ 'issue', diff --git a/.github/scripts/assign-issue-owner.test.mjs b/.github/scripts/assign-issue-owner.test.mjs index ff5695e35b5..e64e500eefc 100644 --- a/.github/scripts/assign-issue-owner.test.mjs +++ b/.github/scripts/assign-issue-owner.test.mjs @@ -149,6 +149,43 @@ describe('assign-issue-owner: owner map', () => { broken.areas[0].owners = [42]; assert.throws(() => loadPolicy(JSON.stringify(broken)), /invalid login/); }); + + it('rejects paths entries that could never route the area', () => { + // startsWith matching can never honour these spellings; accepting them + // would silently unroute the area from PR assignment forever. + for (const paths of [ + ['./packages/core/'], + ['packages//core/'], + ['.github/../packages/core/'], + ['/packages/core/'], + ['packages/core'], + ['packages\\core/'], + [''], + [42], + ]) { + const broken = JSON.parse(ownersRaw); + broken.areas[0].paths = paths; + assert.throws( + () => loadPolicy(JSON.stringify(broken)), + /invalid paths entry/, + ); + } + + const notArray = JSON.parse(ownersRaw); + notArray.areas[0].paths = 'packages/core/'; + assert.throws( + () => loadPolicy(JSON.stringify(notArray)), + /paths must be an array/, + ); + + // An explicitly empty list can never route the area either. + const emptyPaths = JSON.parse(ownersRaw); + emptyPaths.areas[0].paths = []; + assert.throws( + () => loadPolicy(JSON.stringify(emptyPaths)), + /paths must not be empty/, + ); + }); }); describe('assign-issue-owner: skip policy', () => { diff --git a/.github/scripts/assign-pr-owner.mjs b/.github/scripts/assign-pr-owner.mjs new file mode 100644 index 00000000000..244126e92a4 --- /dev/null +++ b/.github/scripts/assign-pr-owner.mjs @@ -0,0 +1,323 @@ +#!/usr/bin/env node +// Assign a PR to one area owner, derived purely from the PR's changed file +// paths. +// +// PR-side companion to assign-issue-owner.mjs. The script never reads PR +// title, body, or comments, so untrusted PR text cannot steer who gets +// assigned. The diff's file paths are matched against the optional `paths` +// list of each area in .github/issue-owners.json; the longest matching +// prefix wins, so a module-level entry overrides the coarser fallback area +// that contains it. The assignee is the area's least loaded eligible +// owner, rotated by PR number on ties — the same load metric and rotation as +// issue assignment. Push access is re-verified against the live collaborator +// API before the write, coverage is re-checked against the live PR +// immediately before it, and the run no-ops once any mapped owner is already +// an assignee or has reviewed, so repeated pushes never stack assignments. +import { appendFileSync, readFileSync } from 'node:fs'; +import { spawnSync } from 'node:child_process'; +import { pathToFileURL } from 'node:url'; + +import { + loadPolicy, + openIssueCount, + pickOwner, +} from './assign-issue-owner.mjs'; + +const OWNERS_FILE = '.github/issue-owners.json'; +const WRITE_PERMISSIONS = new Set(['admin', 'maintain', 'write']); +const BOT_LOGIN = /(\[bot\]|-bot)$/; + +function gh(args) { + const result = spawnSync('gh', args, { + encoding: 'utf8', + maxBuffer: 10 * 1024 * 1024, + }); + if (result.status !== 0) { + throw new Error(result.stderr.trim() || `gh ${args.join(' ')} failed`); + } + return result.stdout.trim(); +} + +function record(lines) { + const body = `${lines.join('\n')}\n`; + process.stdout.write(body); + if (process.env.GITHUB_STEP_SUMMARY) { + appendFileSync(process.env.GITHUB_STEP_SUMMARY, body); + } +} + +export function skipPrReason(pr) { + if (pr.state !== 'OPEN') return 'PR is not open'; + if (pr.isDraft) return 'PR is a draft'; + // `gh pr view --json author` exports `"author": null` once the account is + // deleted; skip gracefully like the other skip reasons instead of throwing + // on the null dereference and failing the check on every later trigger. + const authorLogin = pr.author?.login; + if (!authorLogin) return 'PR author account was deleted'; + if (BOT_LOGIN.test(authorLogin)) return 'authored by a bot'; + return null; +} + +// The longest matching prefix wins, so a module entry overrides the coarser +// area that contains it; ties keep the earlier area in file order. Areas +// without a paths list never match. +export function matchedAreasByPath(policy, files) { + const ranked = []; + for (const area of policy.areas) { + let length = 0; + for (const prefix of area.paths ?? []) { + if (files.some((file) => file.path.startsWith(prefix))) { + length = Math.max(length, prefix.length); + } + } + if (length > 0) ranked.push({ area, length }); + } + ranked.sort((a, b) => b.length - a.length); + return ranked.map((entry) => entry.area); +} + +export function matchAreaByPath(policy, files) { + return matchedAreasByPath(policy, files)[0] ?? null; +} + +// An assignee or a non-dismissed review by any mapped owner means this +// routing already happened; never stack a second assignment. A dismissed +// review is a removed review, so it must not count as coverage. +export function alreadyCovered(policy, pr) { + const pool = new Set( + policy.areas + .flatMap((area) => area.owners) + .map((login) => login.toLowerCase()), + ); + const involved = [ + ...pr.assignees.map((assignee) => assignee.login), + ...pr.latestReviews + .filter((review) => review.state !== 'DISMISSED') + .map((review) => review.author?.login), + ]; + return involved.some((login) => login && pool.has(login.toLowerCase())); +} + +// Same stale-entry tolerance as the issue script: a candidate who lost push +// access is dropped with a warning, not a failed run. +function canWrite(repository, login) { + try { + return WRITE_PERMISSIONS.has( + gh([ + 'api', + `repos/${repository}/collaborators/${login}/permission`, + '--jq', + '.permission', + ]), + ); + } catch (error) { + console.warn( + `::warning::Cannot verify push access for @${login}: ${error.message}`, + ); + return false; + } +} + +// Same warn-and-continue tolerance as canWrite(): the load fan-out is one +// `gh issue list` per eligible owner (up to 15 for the core pool), and one +// transient failure — secondary rate limit, 5xx, issues disabled — must not +// fail the run on the contributor's PR. Retry once, then degrade to a zero +// load so the rotation still lands an owner; the load metric is a tie-break +// heuristic, not a gate on assigning. +function ownerLoad(repository, login) { + let lastError; + for (let attempt = 0; attempt < 2; attempt += 1) { + try { + return openIssueCount(repository, login); + } catch (error) { + lastError = error; + } + } + console.warn( + `::warning::Cannot read open-issue load for @${login}: ${lastError.message}`, + ); + return 0; +} + +// `gh pr view --json files` caps at 100 entries, so the REST files endpoint +// pages through every changed file instead. Each filename is decoded from +// base64 because changed filenames are attacker-controlled on fork PRs and +// git accepts newlines in path components: splitting the rendered text on +// newlines would let a name like "xpackages/core/poc" inject a phantom +// "packages/core/poc" entry that steers which area owner gets assigned. +export function changedFiles(repository, prNumber) { + return gh([ + 'api', + `repos/${repository}/pulls/${prNumber}/files`, + '--paginate', + '--jq', + '.[] | (.filename, (.previous_filename // empty)) | @base64', + ]) + .split('\n') + .filter(Boolean) + .map((line) => ({ + path: Buffer.from(line, 'base64').toString('utf8'), + })); +} + +function main() { + const repository = process.env.GITHUB_REPOSITORY; + const prNumber = Number(process.env.PR_NUMBER); + const dryRun = process.env.DRY_RUN === 'true'; + if (!repository || !/^[^/]+\/[^/]+$/.test(repository)) { + throw new Error('Invalid repository'); + } + if (!Number.isSafeInteger(prNumber) || prNumber < 1) { + throw new Error('Invalid PR number'); + } + + const policy = loadPolicy(readFileSync(OWNERS_FILE, 'utf8')); + const pr = JSON.parse( + gh([ + 'pr', + 'view', + String(prNumber), + '--repo', + repository, + '--json', + 'state,isDraft,author,assignees,latestReviews,headRefOid', + ]), + ); + pr.files = changedFiles(repository, prNumber); + + const skip = skipPrReason(pr); + if (skip) { + record([`Assignment: skipped — ${skip}`]); + return; + } + if (alreadyCovered(policy, pr)) { + record(['Assignment: skipped — a mapped owner is already on the PR']); + return; + } + const matched = matchedAreasByPath(policy, pr.files); + if (matched.length === 0) { + record(['Assignment: skipped — no area path matched']); + return; + } + + // Never assign the PR author to their own work. When a module's owner + // cannot take the PR (they authored it, or lost push access), fall back + // to the next coarser matching area instead of leaving the PR unassigned. + // The null guard mirrors skipPrReason's: a deleted account exports as + // `"author": null` and can exclude no one. + const authorLogin = (pr.author?.login ?? '').toLowerCase(); + let area; + let eligible; + for (area of matched) { + eligible = area.owners.filter( + (owner) => + owner.toLowerCase() !== authorLogin && canWrite(repository, owner), + ); + if (eligible.length > 0) break; + } + if (!eligible || eligible.length === 0) { + console.warn( + `::warning::No eligible owner for the areas touched by this PR; check ${OWNERS_FILE}.`, + ); + record(['Assignment: skipped — no eligible owner for the matched areas']); + return; + } + if (area !== matched[0]) { + record([ + `Area ${matched[0].name} has no eligible owner (author or no push access); falling back to ${area.name}`, + ]); + } + + const loadByOwner = new Map( + eligible.map((owner) => [owner, ownerLoad(repository, owner)]), + ); + const assignee = pickOwner(eligible, loadByOwner, prNumber); + + if (dryRun) { + record([ + `Area: ${area.name}`, + `Assignment: dry-run — would assign @${assignee} (${loadByOwner.get(assignee)} open)`, + ]); + return; + } + + // Between the opening snapshot and this write sit up to ~30 sequential API + // calls (permission and load per candidate), and during that window a + // human or a concurrent run may already have put a mapped owner on the PR + // or closed it. Re-check the live PR immediately before mutating, + // mirroring the sibling issue script's pre-write re-fetch, so a covered PR + // is never assigned twice. + const latestPr = JSON.parse( + gh([ + 'pr', + 'view', + String(prNumber), + '--repo', + repository, + '--json', + 'state,isDraft,author,assignees,latestReviews,headRefOid', + ]), + ); + const latestSkip = skipPrReason(latestPr); + if (latestSkip) { + record([`Assignment: skipped — ${latestSkip}`]); + return; + } + if (latestPr.headRefOid !== pr.headRefOid) { + record(['Assignment: skipped — PR head changed during routing']); + return; + } + if (alreadyCovered(policy, latestPr)) { + record(['Assignment: skipped — a mapped owner is already on the PR']); + return; + } + + try { + gh([ + 'pr', + 'edit', + String(prNumber), + '--repo', + repository, + '--add-assignee', + assignee, + ]); + } catch (error) { + if (/assigning agents is not supported/i.test(error.message)) { + // A PR that already carries a coding-agent assignee can only change + // its actor list through replaceActorsForAssignable, which GitHub + // refuses for GitHub App installation tokens. The PR already has an + // accountable actor, so skip like the other token-limit cases instead + // of failing the check on the contributor's PR. + record([ + 'Assignment: skipped — token cannot assign PRs with agent assignees', + ]); + return; + } + if ( + /permission|403|resource not accessible by integration/i.test( + error.message, + ) + ) { + record(['Assignment: skipped — token cannot assign PRs']); + return; + } + throw error; + } + record([ + `Area: ${area.name}`, + `Assignment: assigned @${assignee} (${loadByOwner.get(assignee)} open)`, + ]); +} + +if ( + process.argv[1] && + import.meta.url === pathToFileURL(process.argv[1]).href +) { + try { + main(); + } catch (error) { + console.error(error instanceof Error ? error.message : String(error)); + process.exit(1); + } +} diff --git a/.github/scripts/assign-pr-owner.test.mjs b/.github/scripts/assign-pr-owner.test.mjs new file mode 100644 index 00000000000..bab5dc009fa --- /dev/null +++ b/.github/scripts/assign-pr-owner.test.mjs @@ -0,0 +1,690 @@ +// Guards for path-driven PR assignment. Load-bearing pieces with no other +// test: the pure functions that decide *whether* and *to whom* a PR is +// assigned, the one-assignment-per-PR idempotency, and the workflow +// invariants that keep the pull_request_target trigger safe (trusted-base +// checkout, repository guard, job-scoped permissions, step-scoped token). +import assert from 'node:assert/strict'; +import { + chmodSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { spawnSync } from 'node:child_process'; +import { afterEach, describe, it } from 'node:test'; +import { fileURLToPath } from 'node:url'; +import { parse } from 'yaml'; + +import { loadPolicy, pickOwner } from './assign-issue-owner.mjs'; +import { + alreadyCovered, + changedFiles, + matchAreaByPath, + matchedAreasByPath, + skipPrReason, +} from './assign-pr-owner.mjs'; + +const scriptsDir = dirname(fileURLToPath(import.meta.url)); +const repoRoot = join(scriptsDir, '..', '..'); +const script = join(scriptsDir, 'assign-pr-owner.mjs'); +const policy = loadPolicy( + readFileSync(join(repoRoot, '.github', 'issue-owners.json'), 'utf8'), +); +const tempDirs = []; + +const corePr = { + state: 'OPEN', + isDraft: false, + headRefOid: 'head-1', + author: { login: 'some-contributor' }, + assignees: [], + latestReviews: [], +}; + +afterEach(() => { + while (tempDirs.length > 0) { + rmSync(tempDirs.pop(), { recursive: true, force: true }); + } +}); + +describe('assign-pr-owner: pure routing', () => { + it('routes module paths to their module area, everything else to the fallback', () => { + const area = matchAreaByPath(policy, [{ path: 'packages/core/src/x.ts' }]); + assert.equal(area?.name, 'core'); + const module = matchAreaByPath(policy, [ + { path: 'packages/core/src/skills/loader.ts' }, + ]); + assert.equal(module?.name, 'core-skills'); + // A module entry overrides the coarser fallback even when the PR also + // touches files only the fallback covers. + const mixed = matchAreaByPath(policy, [ + { path: 'packages/core/src/index.ts' }, + { path: 'packages/core/src/goals/goal.ts' }, + ]); + assert.equal(mixed?.name, 'core-goals'); + }); + + it('routes a probe file under every mapped prefix back to its area', () => { + // Fixed probe prefixes, deliberately independent of the policy: a typo'd + // prefix in issue-owners.json must fail here instead of silently + // rerouting that module's PRs to the generic fallback while the suite + // stays green (the earlier tests only pinned core-skills and core-goals). + // Probes derived from the policy itself would shift with the typo and + // stay green, so the expected prefixes are spelled out. + const probes = { + core: ['packages/core/'], + 'core-skills': ['packages/core/src/skills/'], + 'core-memory': ['packages/core/src/memory/'], + 'core-goals': ['packages/core/src/goals/'], + 'core-telemetry': ['packages/core/src/telemetry/'], + 'core-extension': ['packages/core/src/extension/'], + 'core-agents': ['packages/core/src/agents/'], + 'core-config': ['packages/core/src/config/'], + 'core-runtime': [ + 'packages/core/src/core/', + 'packages/core/src/services/', + 'packages/core/src/tools/', + 'packages/core/src/utils/', + ], + }; + const mapped = policy.areas.filter((area) => area.paths?.length); + for (const area of mapped) { + const prefixes = probes[area.name]; + assert.ok(prefixes, `area ${area.name} has no probe prefixes`); + assert.deepEqual( + area.paths, + prefixes, + `area ${area.name} changed its paths — update the probes`, + ); + for (const prefix of prefixes) { + assert.equal( + matchedAreasByPath(policy, [{ path: `${prefix}routing-probe.ts` }])[0] + ?.name, + area.name, + `probe under ${prefix} does not route to ${area.name}`, + ); + } + } + // Stale probes for areas that lost their paths list must fail too. + assert.deepEqual( + new Set(Object.keys(probes)), + new Set(mapped.map((area) => area.name)), + ); + }); + + it('never matches outside the mapped prefixes', () => { + assert.equal( + matchAreaByPath(policy, [{ path: 'packages/cli/src/x.ts' }]), + null, + ); + // A prefix string must be a directory prefix, not a substring. + assert.equal( + matchAreaByPath(policy, [{ path: 'packages/coredump/x.ts' }]), + null, + ); + }); + + it('skips areas without a paths list instead of matching them', () => { + // paths stays optional in loadPolicy, so a label-only area is valid + // config; path routing must skip it rather than crash on the missing + // list. + const labelOnly = { + name: 'label-only', + labels: ['area: core'], + owners: [policy.areas[0].owners[0]], + }; + const synthetic = { + requireLabels: [], + skipLabels: [], + areas: [labelOnly, ...policy.areas], + }; + const area = matchAreaByPath(synthetic, [ + { path: 'packages/core/src/x.ts' }, + ]); + assert.equal(area?.name, 'core'); + assert.equal( + matchAreaByPath({ ...synthetic, areas: [labelOnly] }, [ + { path: 'packages/core/src/x.ts' }, + ]), + null, + ); + }); + + it('skips closed, draft, and bot-authored PRs', () => { + assert.equal(skipPrReason(corePr), null); + assert.ok(skipPrReason({ ...corePr, state: 'MERGED' })); + assert.ok(skipPrReason({ ...corePr, isDraft: true })); + assert.ok( + skipPrReason({ ...corePr, author: { login: 'qwen-code-ci-bot' } }), + ); + assert.ok( + skipPrReason({ ...corePr, author: { login: 'dependabot[bot]' } }), + ); + // A deleted account exports as "author": null — skip with a reason, + // never throw on the null dereference. + assert.ok(skipPrReason({ ...corePr, author: null })); + }); + + it('treats a mapped assignee or reviewer as covered', () => { + const owner = policy.areas[0].owners[0]; + assert.ok( + alreadyCovered(policy, { ...corePr, assignees: [{ login: owner }] }), + ); + assert.ok( + alreadyCovered(policy, { + ...corePr, + latestReviews: [{ author: { login: owner }, state: 'APPROVED' }], + }), + ); + // Case-insensitively, and only for mapped owners. + assert.ok( + alreadyCovered(policy, { + ...corePr, + assignees: [{ login: owner.toUpperCase() }], + }), + ); + assert.equal( + alreadyCovered(policy, { + ...corePr, + assignees: [{ login: 'random-person' }], + }), + false, + ); + // A dismissed review is a removed review: it must not satisfy the + // coverage gate, or a PR never assigned on open stays ownerless. + assert.equal( + alreadyCovered(policy, { + ...corePr, + latestReviews: [{ author: { login: owner }, state: 'DISMISSED' }], + }), + false, + ); + }); +}); + +// The stub reports the zeroLoadOwner as the least loaded owner so the pick is +// unambiguous regardless of the rotation offset for PR 77. +function runAssign(dryRun, options = {}) { + const { + prJson = JSON.stringify(corePr), + // When set, the second `pr view` (the pre-write re-fetch) sees this PR + // state instead of prJson, so a test can simulate the PR changing while + // the run is in flight. + prLatestJson = '', + files = 'packages/core/src/foo.ts', + // Raw filename list; overrides `files` when a test needs names a + // newline-joined string cannot carry (git allows newlines in paths). + fileList, + previousFiles = [], + zeroLoadOwner = 'DennisYu07', + // What every collaborator permission lookup answers; 'error' fails the + // lookup outright instead of answering. + permission = 'write', + // When set, this one login's lookup answers denyPerm instead, so a test + // can drop a single owner out of the eligible set. + denyLogin = '', + denyPerm = 'read', + editExit = 0, + editErr = '', + // 'once' fails only the first issue-list lookup (the retry and every + // later call succeed); 'always' fails every one. + loadFail = '', + loadErr = 'HTTP 502: Bad Gateway', + expectExit = 0, + } = options; + const fileNames = fileList ?? files.split('\n').filter(Boolean); + const dir = mkdtempSync(join(tmpdir(), 'assign-pr-owner-')); + tempDirs.push(dir); + const log = join(dir, 'gh.log'); + const gh = join(dir, 'gh'); + writeFileSync( + gh, + `#!/bin/sh +printf '%s\\n' "$*" >> "$GH_STUB_LOG" +case "$*" in + "pr view 77 "*) + # The log line for this call is already appended, so the first view + # counts 1 and the pre-write re-fetch counts 2. + if [ -n "$GH_STUB_PR_LATEST" ] && [ "$(grep -c 'pr view' "$GH_STUB_LOG")" -gt 1 ]; then + printf '%s' "$GH_STUB_PR_LATEST" + else + printf '%s' "$GH_STUB_PR" + fi + ;; + *"pulls/77/files"*"previous_filename"*"@base64"*) + printf '%s' "$GH_STUB_FILES_B64" + if [ -n "$GH_STUB_PREVIOUS_FILES_B64" ]; then + printf '\n%s' "$GH_STUB_PREVIOUS_FILES_B64" + fi + ;; + *"pulls/77/files"*"@base64"*) printf '%s' "$GH_STUB_FILES_B64" ;; + *"pulls/77/files"*) printf '%s' "$GH_STUB_FILES" ;; + *"/collaborators/$GH_STUB_DENY_LOGIN/permission"*) printf '%s' "$GH_STUB_DENY_PERM" ;; + *"/collaborators/"*"/permission"*) + if [ "$GH_STUB_PERMISSION" = "error" ]; then + printf '%s' 'permission lookup failed' >&2 + exit 1 + fi + printf '%s' "$GH_STUB_PERMISSION" + ;; + *"issue list"*"--json number"*) + # The log line for this call is already appended, so the first lookup + # counts 1: 'once' fails exactly that one call, and its retry succeeds. + if [ "$GH_STUB_LOAD_FAIL" = "always" ] || { [ "$GH_STUB_LOAD_FAIL" = "once" ] && [ "$(grep -c 'issue list' "$GH_STUB_LOG")" -le 1 ]; }; then + printf '%s' "$GH_STUB_LOAD_ERR" >&2 + exit 1 + fi + case "$*" in + *"--assignee ${zeroLoadOwner}"*) printf '%s' '0' ;; + *) printf '%s' '5' ;; + esac + ;; + "pr edit "*) printf '%s' "$GH_STUB_EDIT_ERR" >&2; exit "$GH_STUB_EDIT_EXIT" ;; +esac +`, + ); + chmodSync(gh, 0o755); + const result = spawnSync(process.execPath, [script], { + cwd: repoRoot, + encoding: 'utf8', + env: { + ...process.env, + PATH: `${dir}:${process.env.PATH}`, + GH_STUB_LOG: log, + GH_STUB_PR: prJson, + GH_STUB_PR_LATEST: prLatestJson, + // What the legacy text-rendering filter would print: one filename per + // line, so an embedded newline forges extra entries. + GH_STUB_FILES: fileNames.join('\n'), + // What the fixed `| @base64` filter prints: one base64 token per + // filename, so an embedded newline stays inside one entry. + GH_STUB_FILES_B64: fileNames + .map((name) => Buffer.from(name, 'utf8').toString('base64')) + .join('\n'), + GH_STUB_PREVIOUS_FILES_B64: previousFiles + .map((name) => Buffer.from(name, 'utf8').toString('base64')) + .join('\n'), + GH_STUB_PERMISSION: permission, + // The stub's deny branch pattern-expands this login; '__none__' can + // never collide with a real collaborator lookup. + GH_STUB_DENY_LOGIN: denyLogin || '__none__', + GH_STUB_DENY_PERM: denyPerm, + GH_STUB_EDIT_EXIT: String(editExit), + GH_STUB_EDIT_ERR: editErr, + GH_STUB_LOAD_FAIL: loadFail, + GH_STUB_LOAD_ERR: loadErr, + GITHUB_REPOSITORY: 'QwenLM/qwen-code', + GITHUB_STEP_SUMMARY: '', + PR_NUMBER: '77', + DRY_RUN: String(dryRun), + }, + }); + assert.equal(result.status, expectExit, result.stderr); + return { + log: readFileSync(log, 'utf8'), + stdout: result.stdout, + stderr: result.stderr, + }; +} + +describe('assign-pr-owner: apply boundary', () => { + it('assigns the least loaded eligible owner', () => { + const { log, stdout } = runAssign(false); + assert.match(log, /pr edit 77 .*--add-assignee DennisYu07/); + assert.match(stdout, /assigned @DennisYu07/); + }); + + it('performs no mutation in dry-run mode', () => { + const { log, stdout } = runAssign(true); + assert.doesNotMatch(log, /pr edit/); + assert.match(stdout, /dry-run — would assign @DennisYu07/); + }); + + it('never assigns the PR author to their own work', () => { + const { log, stdout } = runAssign(false, { + prJson: JSON.stringify({ ...corePr, author: { login: 'DennisYu07' } }), + // Without the exclusion the zero-load author would win outright. + }); + assert.doesNotMatch(log, /--add-assignee DennisYu07\b/); + assert.match(stdout, /assigned @/); + }); + + it('skips gracefully when the PR author account was deleted', () => { + // `gh pr view --json author` exports `"author": null` for deleted + // accounts. Every later trigger (synchronize/reopened/ready_for_review/ + // manual) must skip with a clean exit instead of throwing on the null + // dereference, which would run the assignment check red on every push. + const { log, stdout } = runAssign(false, { + prJson: JSON.stringify({ ...corePr, author: null }), + }); + assert.doesNotMatch(log, /pr edit/); + assert.match(stdout, /skipped — PR author account was deleted/); + }); + + it('no-ops once a mapped owner is already on the PR', () => { + const owner = policy.areas[0].owners[0]; + const { log, stdout } = runAssign(false, { + prJson: JSON.stringify({ + ...corePr, + assignees: [{ login: owner }], + }), + }); + assert.doesNotMatch(log, /pr edit/); + assert.match(stdout, /already on the PR/); + }); + + it('skips when no area path matches', () => { + const { log, stdout } = runAssign(false, { + files: 'packages/cli/src/index.ts', + }); + assert.doesNotMatch(log, /pr edit/); + assert.match(stdout, /no area path matched/); + }); + + it('re-checks coverage against the live PR immediately before the write', () => { + // Up to ~30 sequential API calls sit between the opening snapshot and + // the write; a mapped owner landing on the PR in that window must stop + // the assignment instead of stacking a second one. + const owner = policy.areas[0].owners[0]; + const { log, stdout } = runAssign(false, { + prLatestJson: JSON.stringify({ + ...corePr, + assignees: [{ login: owner }], + }), + }); + assert.doesNotMatch(log, /pr edit/); + assert.match(stdout, /already on the PR/); + }); + + it('skips the write when the PR closes mid-run', () => { + const { log, stdout } = runAssign(false, { + prLatestJson: JSON.stringify({ ...corePr, state: 'MERGED' }), + }); + assert.doesNotMatch(log, /pr edit/); + assert.match(stdout, /skipped — PR is not open/); + }); + + it('skips the write when the PR head changes mid-run', () => { + const { log, stdout } = runAssign(false, { + prLatestJson: JSON.stringify({ ...corePr, headRefOid: 'head-2' }), + }); + assert.equal((log.match(/headRefOid/g) ?? []).length, 2); + assert.doesNotMatch(log, /pr edit/); + assert.match(stdout, /skipped — PR head changed during routing/); + }); + + it('falls back to the coarser area when the module owner authored the PR', () => { + const module = policy.areas.find((area) => area.name === 'core-skills'); + const { log, stdout } = runAssign(false, { + prJson: JSON.stringify({ + ...corePr, + author: { login: module.owners[0] }, + }), + files: 'packages/core/src/skills/loader.ts', + }); + assert.match(stdout, /falling back to core/); + assert.match(log, /pr edit 77 .*--add-assignee DennisYu07/); + }); + + it('drops an owner who lost push access and falls back to the coarser area', () => { + const module = policy.areas.find((area) => area.name === 'core-skills'); + const { log, stdout } = runAssign(false, { + denyLogin: module.owners[0], + files: 'packages/core/src/skills/loader.ts', + }); + assert.match(stdout, /falling back to core/); + assert.doesNotMatch( + log, + new RegExp(`--add-assignee ${module.owners[0]}\\b`), + ); + assert.match(log, /pr edit 77 .*--add-assignee DennisYu07/); + }); + + it('exits without assigning when no owner passes the push-access check', () => { + // 'read' answers every lookup with a non-write permission; 'error' fails + // the lookup outright (the canWrite catch branch). Both routes must land + // on the same terminal skip with a clean exit — never a failed run, and + // never a blind assignment past the collaborator check. + for (const permission of ['read', 'error']) { + const { log, stdout, stderr } = runAssign(false, { permission }); + assert.doesNotMatch(log, /pr edit/); + assert.match(stdout, /no eligible owner for the matched areas/); + if (permission === 'error') { + assert.match(stderr, /Cannot verify push access/); + } + } + }); + + it('tolerates a read-only token instead of failing', () => { + const { log, stdout } = runAssign(false, { + editExit: 1, + editErr: 'HTTP 403: Resource not accessible by integration', + }); + assert.doesNotMatch(stdout, /assigned @/); + assert.match(stdout, /token cannot assign/); + assert.match(log, /pr edit/); + }); + + it('re-throws a non-permission pr edit failure instead of swallowing it', () => { + const { log } = runAssign(false, { + editExit: 1, + editErr: 'network error', + expectExit: 1, + }); + assert.match(log, /pr edit/); + }); + + it('skips gracefully when GitHub refuses agent-assignee edits for App tokens', () => { + // A PR that already carries a coding-agent assignee can only change its + // actor list through replaceActorsForAssignable, which GitHub refuses + // for GitHub App installation tokens. That refusal must land on the + // graceful skip — a rethrow here runs the assignment check red on the + // contributor's own PR (@wenshao's F1). + const { log, stdout } = runAssign(false, { + editExit: 1, + editErr: + 'GraphQL: Assigning agents is not supported with GitHub App installation tokens (HTTP 400)', + }); + assert.doesNotMatch(stdout, /assigned @/); + assert.match( + stdout, + /skipped — token cannot assign PRs with agent assignees/, + ); + assert.match(log, /pr edit/); + }); + + it('tolerates a transient issue-list failure after one retry', () => { + // One transient gh issue list failure hits the first load lookup; the + // retry must recover the real load with no degraded fallback, and the + // extra call must be visible in the log. + const owners = policy.areas.find((area) => area.name === 'core').owners; + const { log, stdout, stderr } = runAssign(false, { loadFail: 'once' }); + assert.equal((log.match(/issue list/g) ?? []).length, owners.length + 1); + assert.doesNotMatch(stderr, /Cannot read open-issue load/); + assert.match(log, /pr edit 77 .*--add-assignee DennisYu07/); + assert.match(stdout, /assigned @DennisYu07 \(0 open\)/); + }); + + it('still assigns through rotation when every issue-list lookup fails', () => { + // @wenshao's second failing run: issues disabled made every load lookup + // throw and failed the check. All loads must degrade with a warning and + // the rotation must still land an owner — the load metric is a + // tie-break heuristic, not a gate on assigning. + const owners = policy.areas.find((area) => area.name === 'core').owners; + const degraded = new Map(owners.map((owner) => [owner, 0])); + const rotated = pickOwner(owners, degraded, 77); + const { log, stdout, stderr } = runAssign(false, { loadFail: 'always' }); + assert.match(stderr, /Cannot read open-issue load/); + assert.match(log, new RegExp(`pr edit 77 .*--add-assignee ${rotated}`)); + assert.match(stdout, new RegExp(`assigned @${rotated}`)); + }); +}); + +describe('assign-pr-owner: untrusted filename decoding', () => { + it('includes both paths for renamed files', () => { + const { log, stdout } = runAssign(false, { + files: 'packages/cli/src/new.ts', + previousFiles: ['packages/core/src/old.ts'], + }); + assert.match(log, /previous_filename/); + assert.match(log, /pr edit 77 .*--add-assignee DennisYu07/); + assert.match(stdout, /Area: core/); + }); + + it('keeps a newline inside a changed filename from forging a second path', () => { + // Changed filenames are attacker-controlled on fork PRs, and git accepts + // newlines in path components. Decoding each filename structurally must + // keep "xpackages/core/poc" one entry; splitting the rendered text + // on newlines would turn it into a phantom "packages/core/poc" that a + // fork author could use to steer which area owner gets assigned. + const forged = 'x\npackages/core/poc'; + const fileNames = [forged, 'packages/core/src/foo.ts']; + + // Decode boundary, probed directly through a stub that renders what the + // real files endpoint would for each jq filter. + const dir = mkdtempSync(join(tmpdir(), 'assign-pr-owner-')); + tempDirs.push(dir); + const ghPath = join(dir, 'gh'); + writeFileSync( + ghPath, + `#!/bin/sh +case "$*" in + *"pulls/77/files"*"@base64"*) printf '%s' '${fileNames + .map((name) => Buffer.from(name, 'utf8').toString('base64')) + .join('\n')}' ;; + *"pulls/77/files"*) printf '%s' '${fileNames.join('\n')}' ;; + *) printf 'unexpected gh call: %s\\n' "$*" >&2; exit 1 ;; +esac +`, + ); + chmodSync(ghPath, 0o755); + const prevPath = process.env.PATH; + process.env.PATH = `${dir}:${prevPath}`; + let decoded; + try { + decoded = changedFiles('QwenLM/qwen-code', 77); + } finally { + process.env.PATH = prevPath; + } + // The forged name arrives as exactly one entry, newline intact. + assert.deepEqual(decoded, [ + { path: forged }, + { path: 'packages/core/src/foo.ts' }, + ]); + // On its own it matches no area prefix... + assert.equal(matchAreaByPath(policy, [decoded[0]]), null); + // ...while the legit core file still routes the PR to core. + assert.equal(matchAreaByPath(policy, decoded)?.name, 'core'); + + // End to end: a PR carrying only the forged name is skipped instead of + // being routed to core through the phantom split entry... + const alone = runAssign(false, { fileList: [forged] }); + assert.doesNotMatch(alone.log, /pr edit/); + assert.match(alone.stdout, /no area path matched/); + // ...and adding a legit core file routes to core for that file's sake. + const mixed = runAssign(false, { fileList: fileNames }); + assert.match(mixed.log, /pr edit 77 .*--add-assignee DennisYu07/); + assert.match(mixed.stdout, /Area: core/); + }); +}); + +const doc = parse( + readFileSync( + join(repoRoot, '.github', 'workflows', 'assign-pr-owner.yml'), + 'utf8', + ), +); +// YAML 1.1 parses the bare key `on` as boolean true. +const triggers = doc.on ?? doc[true]; +const assignJob = doc.jobs.assign; + +describe('assign-pr-owner: workflow invariants', () => { + it('runs only on the canonical repository', () => { + assert.match(assignJob.if, /github\.repository == 'QwenLM\/qwen-code'/); + }); + + it('keeps the privileged pull_request_target trigger and never cancels in flight', () => { + // pull_request_target is the whole safety case: fork PRs get owners + // while the checkout stays on the trusted base. Flipping it to + // pull_request makes the token read-only on fork PRs, and the + // permission-tolerance catch then records a skip — routing silently + // disabled for every fork PR with green checks. Dropping + // ready_for_review leaves drafts ownerless after they are marked + // ready, and cancel-in-progress would kill an in-flight assignment + // mid-write on a synchronize burst. + assert.deepEqual(triggers.pull_request_target.types, [ + 'opened', + 'synchronize', + 'reopened', + 'ready_for_review', + ]); + assert.equal( + doc.concurrency.group, + 'assign-pr-owner-${{ github.event.pull_request.number || inputs.number }}', + ); + assert.equal(doc.concurrency['cancel-in-progress'], false); + }); + + it('scopes the write permission to the job and the token to the step', () => { + assert.equal(doc.permissions['pull-requests'], undefined); + assert.equal(assignJob.permissions['pull-requests'], 'write'); + const runStep = assignJob.steps.find((step) => step.run); + assert.ok(runStep.env.GH_TOKEN); + assert.equal(doc.env?.GH_TOKEN, undefined); + assert.equal( + assignJob.env, + undefined, + 'job-level env exposes GH_TOKEN to every step', + ); + // Same pin as the issue script: a hardcoded or dropped DRY_RUN turns + // event-triggered runs into permanent no-ops, and breaking the + // inputs.number fallback breaks every manual dispatch. + assert.equal( + runStep.env.PR_NUMBER, + '${{ github.event.pull_request.number || inputs.number }}', + ); + assert.equal(runStep.env.DRY_RUN, "${{ inputs.dry_run || 'false' }}"); + }); + + it('checks out the trusted base, credential-free and sparse', () => { + const checkout = assignJob.steps.find((step) => + step.uses?.startsWith('actions/checkout@'), + ); + assert.match(checkout.with.ref, /pull_request\.base\.sha/); + assert.equal(checkout.with['persist-credentials'], false); + assert.match(checkout.with['sparse-checkout'], /issue-owners\.json/); + // The run step's guard skips when this entry is dropped, so pin the + // membership — otherwise routing could be silently disabled forever. + assert.match( + checkout.with['sparse-checkout'], + /^\.github\/scripts\/assign-pr-owner\.mjs$/m, + ); + // The entry script statically imports assign-issue-owner.mjs, and the + // bootstrap guard only checks for assign-pr-owner.mjs — dropping this + // entry makes node fail on the missing module after the guard passed. + assert.match( + checkout.with['sparse-checkout'], + /^\.github\/scripts\/assign-issue-owner\.mjs$/m, + ); + // Nothing from the PR head can execute: the checkout never follows it. + assert.doesNotMatch(checkout.with.ref, /head\.sha/); + }); + + it('bootstrap-skips on a base without the script, before running node', () => { + const runStep = assignJob.steps.find((step) => step.run); + // Pin the guard's shape and ordering: an inverted guard turns every run + // into a silent no-op, a non-zero exit re-breaks the bootstrap PR's own + // check, and a node call ahead of the guard fails on the base checkout. + assert.match( + runStep.run, + /if \[ ! -f \.github\/scripts\/assign-pr-owner\.mjs \]; then[\s\S]*?exit 0[\s\S]*?fi[\s\S]*?node \.github\/scripts\/assign-pr-owner\.mjs\s*$/, + ); + }); + + it('defaults a manual dispatch to dry-run', () => { + assert.equal(triggers.workflow_dispatch.inputs.dry_run.default, true); + }); +}); diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs index ad2a9cf44ea..62492673c46 100644 --- a/.github/scripts/qwen-triage-workflow.test.mjs +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -693,6 +693,33 @@ describe('ci.yml: self-hosted checkout jobs restore ownership unconditionally', }); }); +describe('qwen-triage: maintainer resolver heredoc vs the backtick deny rule', () => { + it('keeps the resolver bash block backtick-free so the deny rule never fires', () => { + // The triage lane's permissions.deny includes run_shell_command(*`*): + // any command text containing a backtick is EXECUTION_DENIED before + // approval. A template literal (or a backticked comment) inside the + // resolver heredoc would therefore kill the whole command, and + // deferrals would silently lose their deterministic assignee. + const settings = assertSettingsContract(triageStep, 'triage settings'); + const deny = settings.permissions?.deny ?? []; + assert.ok( + deny.includes('run_shell_command(*`*)'), + 'permissions.deny must keep the backtick rule', + ); + const blocks = [...prSkill.matchAll(/```bash\n([\s\S]*?)\n```/g)].map( + (m) => m[1], + ); + const resolver = blocks.find( + (b) => b.includes("<<'EOF'") && b.includes('collaborators'), + ); + assert.ok(resolver, 'maintainer resolver heredoc block must exist'); + assert.ok( + !resolver.includes('`'), + 'resolver block must contain no backtick, or the deny rule blocks it', + ); + }); +}); + describe('qwen-code-pr-review.yml: ownership recovery is unconditional', () => { it('restores ownership without probe gating or rename-aside', () => { assertUnconditional( diff --git a/.github/workflows/.size-baseline b/.github/workflows/.size-baseline index 515a080ccf6..c7b918f5288 100644 --- a/.github/workflows/.size-baseline +++ b/.github/workflows/.size-baseline @@ -13,6 +13,7 @@ # sibling .md and long steps into .github/scripts/ first; if the growth is # real, bump the number and say why in the PR. 2226 assign-issue-owner.yml +2125 assign-pr-owner.yml 3480 audio-capture-prebuilds.yml 9023 auto-minimize-spam.yml 9256 build-and-publish-image.yml diff --git a/.github/workflows/assign-pr-owner.yml b/.github/workflows/assign-pr-owner.yml new file mode 100644 index 00000000000..c6b4846941b --- /dev/null +++ b/.github/workflows/assign-pr-owner.yml @@ -0,0 +1,62 @@ +name: 'Assign PR owner' + +# Assign one area owner to a PR once it touches that area's paths. Routing is +# a pure function of the changed file paths and .github/issue-owners.json — +# no model runs here and the script never reads PR title, body, or comments. +# Runs on pull_request_target so fork PRs are covered too; the checkout stays +# on the trusted base ref and the script only reads the PR's file list from +# the API, so PR content never executes here. + +on: + pull_request_target: + types: ['opened', 'synchronize', 'reopened', 'ready_for_review'] + workflow_dispatch: + inputs: + number: + description: 'PR number to evaluate' + required: true + type: 'string' + dry_run: + description: 'Report the chosen assignee without assigning' + required: false + default: true + type: 'boolean' + +permissions: + contents: 'read' + +concurrency: + group: 'assign-pr-owner-${{ github.event.pull_request.number || inputs.number }}' + cancel-in-progress: false + +jobs: + assign: + if: "${{ github.repository == 'QwenLM/qwen-code' }}" + runs-on: 'ubuntu-latest' + timeout-minutes: 5 + permissions: + contents: 'read' + issues: 'read' + pull-requests: 'write' + steps: + - name: 'Checkout owner map' + uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 + with: + ref: '${{ github.event.pull_request.base.sha || github.sha }}' + sparse-checkout: |- + .github/issue-owners.json + .github/scripts/assign-issue-owner.mjs + .github/scripts/assign-pr-owner.mjs + persist-credentials: false + + - name: 'Assign area owner' + env: + GH_TOKEN: '${{ github.token }}' + PR_NUMBER: '${{ github.event.pull_request.number || inputs.number }}' + DRY_RUN: "${{ inputs.dry_run || 'false' }}" + run: |- + if [ ! -f .github/scripts/assign-pr-owner.mjs ]; then + echo 'Assignment: skipped — trusted base does not contain assign-pr-owner.mjs yet' + exit 0 + fi + node .github/scripts/assign-pr-owner.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d133f3f53f2..89fa161444f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -79,7 +79,7 @@ env: # BOTH the github_ci_only helper step and the full-profile Test step, so a # new helper test can't be added to one path and silently dropped from the # other. - HELPER_TESTS: '.github/scripts/pr-safety-precheck.test.mjs .github/scripts/cap-release-notes.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/ci/classify-platform-sensitivity.test.mjs .github/scripts/ci/classify-pr-profile.test.mjs .github/scripts/upsert-bot-comment.test.mjs .github/scripts/ci/main-failure-signature.test.mjs .github/scripts/classify-release-notes.test.mjs .github/scripts/dsw-swe-verified/make-manifest.test.mjs .github/scripts/dsw-swe-verified/make-terminal-bench-manifest.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs .github/scripts/serve-ab-diff.test.mjs .github/scripts/serve-ab-drive.test.mjs .github/scripts/qwen-triage-workflow.test.mjs .github/scripts/assign-issue-owner.test.mjs .github/scripts/auto-minimize-spam.test.mjs .github/scripts/ci-runner-routing.test.mjs .github/scripts/autofix-status-heartbeat.test.mjs' + HELPER_TESTS: '.github/scripts/pr-safety-precheck.test.mjs .github/scripts/cap-release-notes.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/ci/classify-platform-sensitivity.test.mjs .github/scripts/ci/classify-pr-profile.test.mjs .github/scripts/upsert-bot-comment.test.mjs .github/scripts/ci/main-failure-signature.test.mjs .github/scripts/classify-release-notes.test.mjs .github/scripts/dsw-swe-verified/make-manifest.test.mjs .github/scripts/dsw-swe-verified/make-terminal-bench-manifest.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs .github/scripts/serve-ab-diff.test.mjs .github/scripts/serve-ab-drive.test.mjs .github/scripts/qwen-triage-workflow.test.mjs .github/scripts/assign-issue-owner.test.mjs .github/scripts/auto-minimize-spam.test.mjs .github/scripts/ci-runner-routing.test.mjs .github/scripts/autofix-status-heartbeat.test.mjs .github/scripts/assign-pr-owner.test.mjs' # The growth ratchet and its vitest mirror compare each workflow against # the PR's base commit to tell "this PR grew the file" apart from "the # baseline went stale on main" (#9904). Wired once here so every lane diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index bac83598f6d..3eecc3efdfe 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -892,14 +892,89 @@ Reflection shows it shouldn't merge — request changes immediately, citing the gh pr review "$PR_NUMBER" --repo "$REPO" --request-changes --body "Needs some rethinking — see my notes above. 🙏" ``` -Genuinely unsure, or `GUARD` blocked approval — **don't approve or reject**, but **never defer silently**. Post an explicit defer comment that: +Genuinely unsure, or `GUARD` blocked approval — **don't approve or reject**, but **never defer silently**. Resolve who owns the call, assign the PR to them, and post an explicit defer comment that: 1. States you are escalating to the maintainer. 2. Names the specific reason(s) for uncertainty — what you cannot resolve from the diff, tests, and PR description. -3. @mentions the maintainer (use `$QWEN_MAINTAINER_HANDLE` if set, or the most recent human reviewer). +3. @mentions that maintainer. + +Resolve the maintainer deterministically — never eyeball it. `$QWEN_MAINTAINER_HANDLE` wins when set; otherwise the same owner map and load/rotation logic as issue assignment picks one accountable owner from the PR's labels. The resolver prints one login or nothing, and nothing means "fall through", never "guess": + +```bash +MAINTAINER="${QWEN_MAINTAINER_HANDLE:-}" +if [ -z "$MAINTAINER" ]; then + MAINTAINER=$(REPO="${REPO:-}" PR_NUMBER="${PR_NUMBER:-}" node --input-type=module <<'EOF' 2>/dev/null +import { readFileSync } from 'node:fs'; +import { spawnSync } from 'node:child_process'; +import { loadPolicy, matchArea, openIssueCount, pickOwner } from './.github/scripts/assign-issue-owner.mjs'; + +const gh = (args) => { + const r = spawnSync('gh', args, { encoding: 'utf8' }); + if (r.status !== 0) throw new Error(r.stderr.trim() || 'gh failed'); + return r.stdout.trim(); +}; + +// No lane exports REPO: the triage agent step exports REPOSITORY and Actions +// always provides GITHUB_REPOSITORY, so fall through the skill's documented +// resolve chain. The number arrives as PR_NUMBER or ISSUE_NUMBER depending +// on the lane. Empty/unset values fall through the || chain. +const repo = process.env.REPO || process.env.REPOSITORY || process.env.GITHUB_REPOSITORY; +const prNumber = process.env.PR_NUMBER || process.env.ISSUE_NUMBER; +const pr = JSON.parse(gh(['pr', 'view', prNumber, '--repo', repo, '--json', 'author,labels'])); +const policy = loadPolicy(readFileSync('.github/issue-owners.json', 'utf8')); +const area = matchArea(policy, pr); +if (!area) process.exit(0); + +const canWrite = (login) => { + try { + return ['admin', 'maintain', 'write'].includes( + gh(['api', 'repos/' + repo + '/collaborators/' + login + '/permission', '--jq', '.permission']), + ); + } catch { + return false; + } +}; +// A null author means the account was deleted — nobody to exclude, and +// dereferencing it would throw, letting 2>/dev/null silently bypass the +// deterministic resolver (the jq fallback below already defends this shape). +const authorLogin = pr.author?.login?.toLowerCase() ?? ''; +const eligible = area.owners.filter( + (owner) => owner.toLowerCase() !== authorLogin && canWrite(owner), +); +if (eligible.length === 0) process.exit(0); + +// Same load metric as issue assignment — reuse the exported counter instead +// of re-implementing it, so the two assignment paths cannot drift. +const load = new Map( + eligible.map((owner) => [owner, openIssueCount(repo, owner)]), +); +console.log(pickOwner(eligible, load, Number(prNumber))); +EOF + ) +fi +if [ -z "$MAINTAINER" ]; then + # Last resort: the most recent human reviewer, if any. latestReviews is + # not recency-sorted and bot accounts submit formal reviews here, so + # drop null authors first (a deleted account exports as "author": null, + # and one null login piped into endswith() aborts the whole filter, + # emptying the mention even when a live human reviewer exists), then + # filter the bot-suffix logins and order by submittedAt before taking + # the last — otherwise a defer escalation @mentions a bot and notifies + # nobody. + MAINTAINER=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json latestReviews \ + --jq '[.latestReviews[] | select(.author.login != null) | select((.author.login | (endswith("[bot]") or endswith("-bot"))) | not)] | sort_by(.submittedAt) | last | .author.login // empty') +fi +if [ -n "$MAINTAINER" ]; then + # Put the PR in their Assigned filter — a stronger signal than the mention + # alone. Best-effort: a failed assign must never block the defer comment. + gh pr edit "$PR_NUMBER" --repo "$REPO" --add-assignee "$MAINTAINER" || true +fi +``` + +The heredoc resolves the repository as `REPO` → `REPOSITORY` → `GITHUB_REPOSITORY` and the PR number as `PR_NUMBER` → `ISSUE_NUMBER` (first set wins; the invocation line above passes the session's shell variables through, because an unexported variable never reaches the node child process and `2>/dev/null` would swallow the failure), and it resolves its relative import against the repository root, so run it from the workspace root like every other step here. If nothing resolves — no handle set, no area label on the PR, no eligible owner, no human reviewer — post the comment without an @mention rather than guessing a login. ```bash -gh pr comment "$PR_NUMBER" --repo "$REPO" --body "⏸️ Deferring to @$QWEN_MAINTAINER_HANDLE — . Needs a human call on this one." +gh pr comment "$PR_NUMBER" --repo "$REPO" --body "⏸️ Deferring to @ — . Needs a human call on this one." ``` A defer without an explicit comment is invisible — the maintainer won't know they're needed.