diff --git a/changelog.d/637-mention-trigger.added.md b/changelog.d/637-mention-trigger.added.md new file mode 100644 index 00000000..1cff1f23 --- /dev/null +++ b/changelog.d/637-mention-trigger.added.md @@ -0,0 +1 @@ +A comment that mentions the bot (`@fledge` by default; `FLEDGE_MENTION_NAMES` changes the names) now starts work: a mention on a pull request starts a review of that pull request at its head, and a mention on an issue starts a follow-up for the issue that may open one branch and one pull request. A second mention at the same head does not create a second job, a newer mention replaces a queued job that has not started, and a running job is never interrupted — it finishes first. The trigger uses the same trusted-author filter and per-comment dedupe as the `/fledge` commands, and records the comment, the head SHA, the actor and the outcome. diff --git a/docs/architecture.md b/docs/architecture.md index 14c2d358..9cc6d08e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1448,9 +1448,15 @@ maintainer can see what the agent was told. The bridge prompt tells the agent to push to the same branch and update the existing PR, never open a second PR. The follow-up is deduped by delivery id and by repo + PR + head SHA + a fingerprint of the reason with that instruction, and is skipped while the repository has an -active job; requests are parked per fingerprint, so two different reviews on one -commit are two follow-ups rather than one overwritten entry. A follow-up does not -receive a second automation comment; the job list labels it "Follow-up". +active job. When the running job is a review of the same pull request and head, +updates merge into ONE parked request — one per pull request and head — and each +contributes its own trigger (kind, comment id, actor); the release pass then +queues one job whose `Reviewer asked:` line names every request, so two updates +that arrive before the agent's first model call become one job, not two. A +running review is never cancelled: an update that arrives after that point is not +injected into its current turn, it waits in the same merged request and continues +the session in the next task. A follow-up does not receive a second automation +comment; the job list labels it "Follow-up". ### Default-branch failure intake @@ -1539,6 +1545,36 @@ manufacture work. Each action reuses the existing audit strings (`job.retried`, `job.paused`, `job.resumed`, `job.cancelled`) and their timeline kinds rather than adding a second naming scheme. +### Mention triggers + +`@fledge` in a comment is a second trigger on the same surface, not a second +router. `parseMentionTrigger` sits beside `parseIssueCommand`; it takes the names +from `FLEDGE_MENTION_NAMES` (default `fledge`), matches case-insensitively at a +word boundary, and reuses the command path's trusted set +(`OWNER`/`MEMBER`/`COLLABORATOR`), its created-only rule and its claim keyed by +the comment id, so a replayed delivery starts nothing. The bot's own comment is +never a trigger, and neither is an edited, deleted or closed one. + +The kind of job follows the target's kind. A mention on a pull request starts a +`review` of that pull request at its head: the job reads the diff and comments +and never writes to the branch. A mention on an issue starts a follow-up for +that issue, which may cut one branch and open one pull request under the +one-issue-one-branch-one-PR rule. `Jobs.mention` decides, and the rule itself is +`src/lib/mentionGovernance.ts` so every case is a pure function. + +A mention is identified by `(target kind, target number, head sha)`. The same +target at the same head never opens a second job: while one is queued or running +the mention is recorded as a `job.mention_recorded` event on that job. A newer +mention supersedes a **queued** job — the queued row ends `superseded`, and one +`trigger.mention_superseded` row names both — because that job has not run. A +**running** job is never interrupted: the mention is recorded on it and parks in +`pending_mention:`, and the newest parked mention starts when the slot +frees. A draft pull request or a closed target writes `trigger.mention_ignored`; +a target the installation cannot read writes one `trigger.mention_refused` row +and starts nothing. Every accepted trigger writes `trigger.mention` with the +comment id, the head SHA, the actor and the outcome (`started`, `recorded`, +`parked`), and answers in the thread. + The inbox is also available as `GET /api/inbox/events`, a per-user SSE stream. The server reuses the cached inbox snapshot, sends the server-rendered inbox HTML when the question signature changes, and the browser replaces the section and diff --git a/docs/observability.md b/docs/observability.md index 0e29cd56..894979aa 100644 --- a/docs/observability.md +++ b/docs/observability.md @@ -87,11 +87,13 @@ and the bridge success rate. The GitHub 429 rate and the open-alert count read a real `0` on the same window and keep printing it. The job success rate counts only `succeeded` and `failed` jobs. The other terminal -states — `cancelled`, `paused` and `quota_exhausted` — are excluded from the -numerator and from the denominator alike, so the percentage is a rate over jobs -that ran to success or failure, not over every job Fledge accepted: a surge of -quota refusals or cancellations shrinks that population without moving the rate, -and the report does not show how many jobs it left out. The set itself is decided +states — `cancelled`, `paused`, `quota_exhausted` and `superseded` — are excluded +from the numerator and from the denominator alike, so the percentage is a rate +over jobs that ran to success or failure, not over every job Fledge accepted: a +surge of quota refusals, cancellations or superseded queued jobs shrinks that +population without moving the rate, and the report does not show how many jobs it +left out. A `superseded` job is one a newer trigger replaced before it started, so +it neither failed nor ran. The set itself is decided in [decisions/0008](decisions/0008-slo-denominator.md). ## Traces diff --git a/src/app.ts b/src/app.ts index 97cb1603..4edfa645 100644 --- a/src/app.ts +++ b/src/app.ts @@ -51,7 +51,7 @@ import { allowedRepoRoles, parseRepoRole, roleAllowed } from './lib/repoRole.js' import { isWorkItemState, MAINTAINER_ONLY_STATES, WORK_ITEM_TRANSITIONS, type WorkItemState } from './lib/workItems.js' import { parseStageMode } from './lib/stages.js' import { issuePriority, issueState, shouldTriage } from './lib/issueIntake.js' -import { parseIssueCommand } from './lib/issueCommands.js' +import { parseIssueCommand, parseMentionTrigger, type IssueCommandInput, type MentionTriggerInput } from './lib/issueCommands.js' import { VERSION } from './version.js' import { clearGauge, incCounter, observeHistogram, renderMetrics, setGauge } from './lib/metrics.js' import { readReleaseStatus } from './lib/releaseStatus.js' @@ -118,7 +118,7 @@ import { isRulePath, MAX_RULE_FILE_BYTES } from './rules.js' import { audienceVisible, memoryDigest, parseMemoryInput, searchMemories, visibleMemories } from './lib/memory.js' import { DENSITY_COOKIE, isDensity, resolveDensity } from './views/density.js' import { THEME_COOKIE, isTheme, resolveTheme, withTheme } from './views/theme.js' -import { GithubClient, parseRepoInput, prepareRepo, prepareRepoWithInstallation, repoPermissionWithToken } from './repos/github.js' +import { GithubClient, GithubError, parseRepoInput, prepareRepo, prepareRepoWithInstallation, repoPermissionWithToken } from './repos/github.js' import { checkWebhookSignature, GithubApp, webhookSecretSlot } from './repos/app.js' import { repoGithubToken } from './repos/token.js' import { InstanceManager } from './flybot/instances.js' @@ -5440,9 +5440,17 @@ export async function buildApp(options: BuildOptions): Promise { const repo = store.findRepoByInstallationAndFullName(installationId, fullName) if (repo === undefined) return const parsed = parseIssueCommand(body) - if (parsed === null) return - const { command, issueUrl, number, title, detail } = parsed - const key = `issue_command:${parsed.commentId}` + // #637: a mention is the second matcher on the same comment surface. It is + // read only when the comment is not a `/fledge` command, so one comment can + // never be both, and it reuses the claim below: the comment id is the key. + const mention = parsed === null + ? parseMentionTrigger(body, { names: config.triggers.mentions, botLogin: `${config.githubApp.slug}[bot]` }) + : null + if (parsed === null && mention === null) return + const source = (parsed ?? mention) as IssueCommandInput | MentionTriggerInput + const command = parsed?.command + const { issueUrl, number, title, detail } = source + const key = `issue_command:${source.commentId}` const marker = readIssueCommandMarker(store.getSetting(key)) // Answered already: a redelivery says nothing twice. if (marker?.state === 'done') return @@ -5462,6 +5470,60 @@ export async function buildApp(options: BuildOptions): Promise { store.setSetting(key, `done:${nowIso()}`) } + if (mention !== null) { + // #637: the kind of job follows the target's kind. A pull request is read, + // not written: the review job reads the diff at its head and comments. An + // issue is where a follow-up may open one branch and one pull request. + if (!mention.onPullRequest) { + const result = await jobs.mention(repo, { + target: { kind: 'issue', number: mention.number, url: mention.issueUrl, headSha: '', state: 'open', draft: false }, + commentId: mention.commentId, + actor: mention.actor, + }) + if (result.ok && (result.action === 'start' || result.action === 'supersede')) await reply(`Mention accepted: starting a follow-up for this issue (${result.jobId}).`) + else if (result.ok && result.action === 'record') await reply('Mention recorded on the job that already holds this issue; nothing new was started.') + else if (result.ok) await reply('Mention recorded; the follow-up starts when the job that is running now finishes.') + else await reply(`Mention received, but nothing started: ${result.error}`) + return + } + try { + const pr = await client.getPullRequest(owner, name, mention.number) + const result = await jobs.mention(repo, { + target: { + kind: 'pull_request', + number: mention.number, + url: pr.url, + headSha: pr.headSha, + state: pr.state === 'open' && !pr.merged ? 'open' : 'closed', + draft: pr.draft, + branch: pr.head, + baseBranch: pr.base, + }, + commentId: mention.commentId, + actor: mention.actor, + }) + if (result.ok && (result.action === 'start' || result.action === 'supersede')) await reply(`Mention accepted: review started for ${pr.headSha.slice(0, 7)} on ${pr.url}.`) + else if (result.ok && result.action === 'record') await reply('This head is already being reviewed; the mention is recorded on that job.') + else if (result.ok) await reply('Mention recorded; the review starts when the job that is running now finishes.') + else if (result.code === 'MENTION_DRAFT') await reply('This pull request is a draft; nothing was started.') + else if (result.code === 'MENTION_CLOSED') await reply('This pull request is not open; nothing was started.') + else await reply(`Mention received, but nothing started: ${result.error}`) + return + } catch (error) { + // A target we cannot read starts nothing and says so in one audit line; + // the claim settles, because a redelivery would fail the same read. + if (error instanceof GithubError && (error.status === 404 || error.status === 403)) { + store.audit(repo.user_id, 'trigger.mention_refused', `${mention.commentId} unreadable ${mention.number}`) + store.setSetting(key, `done:${nowIso()}`) + return + } + // Anything else is not the trigger's fault: the claim is released so the + // redelivery can retry it. + store.deleteSetting(key) + throw error + } + } + // #540 slice 6: the chat half. An issue comment is a third surface on the same // command contract as the console forms, so the reply is built from the receipt // and the audit row, the revision fence and the refusal codes are the same ones. @@ -5470,13 +5532,13 @@ export async function buildApp(options: BuildOptions): Promise { // the claim absorbs a duplicate delivery, and if a delivery is retaken the // receipt answers the replay instead of queueing a second job. const runChatCreate = async (kind: JobKind, goal: string, itemId: string): Promise => { - const actor = managementActor({ userId: repo.user_id, repo, channel: 'chat', requestId: `issue_command:${parsed.commentId}` }) + const actor = managementActor({ userId: repo.user_id, repo, channel: 'chat', requestId: `issue_command:${source.commentId}` }) return management.run(actor, { operation: 'task.create', target: { projectId: repo.id }, arguments: { kind, goal, workItemId: itemId }, expectedRevision: management.revisionOf('project', repo.id), - idempotencyKey: `issue_command:${parsed.commentId}`, + idempotencyKey: `issue_command:${source.commentId}`, }) } diff --git a/src/config.ts b/src/config.ts index 1da9e599..f16eab81 100644 --- a/src/config.ts +++ b/src/config.ts @@ -37,6 +37,12 @@ export interface Config { // #708: the per-class windows mutating routes take. A route names its class; // the numbers live here so a deployment tightens a limit without a rebuild. rateLimit: RateLimitBudgets + // #637: the comment names that count as a trigger. `@` in an issue or + // pull request comment starts a review; a deployment renames or adds names + // here rather than in code, and an empty list disables the trigger. + triggers: { + mentions: string[] + } // #540: how far the console adapters go for commands added after slice 1. // `off` (the default) is the route that existed before the contract. managementService: ManagementServiceMode @@ -300,6 +306,12 @@ export function loadConfig(overrides: Partial = {}): Config { windowMs: Math.max(1_000, number('RATE_LIMIT_EXPENSIVE_WINDOW_MS', DEFAULT_RATE_LIMIT_BUDGETS.expensive.windowMs)), }, }, + triggers: { + mentions: text('FLEDGE_MENTION_NAMES', 'fledge') + .split(',') + .map((name) => name.trim()) + .filter((name) => name !== ''), + }, managementService: parseManagementService(text('MANAGEMENT_SERVICE', 'off')), tokenEncryptionKey: encryptionKey, github: { diff --git a/src/db.ts b/src/db.ts index d3126027..da53c90a 100644 --- a/src/db.ts +++ b/src/db.ts @@ -59,7 +59,7 @@ const { DatabaseSync } = require('node:sqlite') as { DatabaseSync: new (path: st export type UserStatus = 'onboarding' | 'pending' | 'approved' | 'rejected' export type UserRole = 'user' | 'admin' | 'superadmin' -export type JobStatus = 'queued' | 'running' | 'succeeded' | 'failed' | 'cancelled' | 'paused' | 'quota_exhausted' +export type JobStatus = 'queued' | 'running' | 'succeeded' | 'failed' | 'cancelled' | 'paused' | 'quota_exhausted' | 'superseded' // #361: a repository's review history, read from rows that already exist. // A window is one `since`; the page asks for two. The outcome of a review is diff --git a/src/jobs/service.ts b/src/jobs/service.ts index 8a7eaebe..f27f904b 100644 --- a/src/jobs/service.ts +++ b/src/jobs/service.ts @@ -43,6 +43,7 @@ import { import { evaluateTestEvidence, type VerificationResult } from '../lib/verification.js' import { observedAttribution } from '../lib/teamStats.js' import { reviewInstructionLine } from '../lib/followUpInstruction.js' +import { decideMention, mentionIdentity, mentionJobKind, type MentionTarget } from '../lib/mentionGovernance.js' import { sweepExpiredTrials, type TrialSweepResult } from '../users/trial.js' import { decryptSecret, encryptSecret, randomId, randomToken, safeEqual, sha256Hex } from '../lib/secrets.js' import { addMinutes, nowIso } from '../lib/time.js' @@ -209,6 +210,107 @@ function followUpFingerprint(reason: string, instruction: string | undefined): s return createHash('sha256').update(`${reason}\n${instruction ?? ''}`).digest('hex').slice(0, 16) } +// #637: a trigger is the update that asked for the review. It travels with the +// request so a merged park can say who asked, not just what was asked. +export interface FollowUpTrigger { + kind: string + commentId?: string + actor?: string +} + +export interface FollowUpRequest { + prUrl: string + branch: string + baseBranch?: string + reason: string + headSha: string + instruction?: string + trigger?: FollowUpTrigger +} + +// #637: one mention, as the webhook read it. The target is what decides the kind +// of job, and `commentId` is what makes two deliveries of the same comment one +// trigger rather than two. +export interface MentionRequest { + target: MentionTarget + commentId: string + actor: string +} + +export type MentionResult = + | { ok: true; action: 'start'; jobId: string; kind: 'review' | 'iterate' } + // Superseded: the queued job ended and this one started in its place. + | { ok: true; action: 'supersede'; jobId: string; kind: 'review' | 'iterate' } + | { ok: true; action: 'record'; jobId: string } + | { ok: true; action: 'park'; jobId: string } + | { ok: false; action: 'refuse' | 'start'; code: string; error: string } + +interface ParkedFollowUpRequest { + reason: string + instruction?: string + trigger?: FollowUpTrigger +} + +// One pull request at one head has ONE parked request, whatever arrived while the +// review was running: the second update merges into the first, and the release +// pass then queues one job carrying both. Keying the park by the request's +// fingerprint turned two updates into two rows and later two jobs. +// #637: the identity a mention job was started for, so a second mention with the +// same identity is recognised without reading the job's goal. +function mentionJobKey(jobId: string): string { + return `mention_job:${jobId}`.slice(0, 300) +} + +// One parked mention per repository: the newest one is what starts next, so a +// second mention replaces the first instead of queueing behind it. +function pendingMentionKey(repoId: string): string { + return `pending_mention:${repoId}`.slice(0, 300) +} + +function pendingFollowUpKey(repoId: string, prUrl: string, headSha: string): string { + return `pending_followup:${repoId}:${prUrl}:${headSha}`.slice(0, 300) +} + +function mergeParkedFollowUp(existingJson: string | undefined, input: FollowUpRequest): string { + const requests: ParkedFollowUpRequest[] = [] + if (existingJson !== undefined) { + try { + const parsed = JSON.parse(existingJson) as Partial & { requests?: ParkedFollowUpRequest[] } + if (Array.isArray(parsed.requests)) { + for (const request of parsed.requests) requests.push({ reason: request.reason, ...(request.instruction === undefined ? {} : { instruction: request.instruction }), ...(request.trigger === undefined ? {} : { trigger: request.trigger }) }) + } else if (typeof parsed.reason === 'string') { + requests.push({ reason: parsed.reason, ...(parsed.instruction === undefined ? {} : { instruction: parsed.instruction }), ...(parsed.trigger === undefined ? {} : { trigger: parsed.trigger }) }) + } + } catch { + // A corrupt parked row is replaced by the request in hand: losing the + // trigger that is arriving is worse than losing a row nobody can read. + } + } + const incoming: ParkedFollowUpRequest = { + reason: input.reason, + ...(input.instruction === undefined ? {} : { instruction: input.instruction }), + ...(input.trigger === undefined ? {} : { trigger: input.trigger }), + } + const duplicate = requests.some((request) => + request.reason === incoming.reason && + (request.instruction ?? '') === (incoming.instruction ?? '') && + (request.trigger?.commentId ?? '') === (incoming.trigger?.commentId ?? '')) + if (!duplicate) requests.push(incoming) + const reasons = [...new Set(requests.map((request) => request.reason))] + const instructions = requests.flatMap((request) => (request.instruction === undefined || request.instruction === '' ? [] : [request.instruction])) + const merged = { + prUrl: input.prUrl, + branch: input.branch, + ...(input.baseBranch === undefined ? {} : { baseBranch: input.baseBranch }), + reason: reasons.join(', '), + headSha: input.headSha, + ...(instructions.length === 0 ? {} : { instruction: instructions.join(' — also: ') }), + ...(input.trigger === undefined ? {} : { trigger: input.trigger }), + requests, + } + return JSON.stringify(merged) +} + export interface ReviewFollowUp { prUrl: string branch: string @@ -2537,7 +2639,7 @@ export class Jobs { const prefix = `pending_followup:${repoId}:` for (const row of this.store.listSettings(prefix)) { this.store.deleteSetting(row.key) - let input: { prUrl: string; branch: string; baseBranch?: string; reason: string; headSha: string; instruction?: string } + let input: FollowUpRequest try { input = JSON.parse(row.value) as typeof input } catch { @@ -2746,7 +2848,7 @@ export class Jobs { } } - async followUp(repo: RepoRow, input: { prUrl: string; branch: string; baseBranch?: string; reason: string; headSha: string; instruction?: string }): Promise { + async followUp(repo: RepoRow, input: FollowUpRequest): Promise { const baseBranch = input.baseBranch ?? (repo.target_branch !== '' ? repo.target_branch : (repo.upstream_default_branch || repo.default_branch)) if (!isValidBranchName(input.branch) || input.branch === baseBranch || input.branch === repo.upstream_default_branch || input.branch === repo.default_branch) { return { ok: false, error: 'follow-up 分支不能是目标分支。', code: 'BRANCH_INVALID' } @@ -2759,8 +2861,8 @@ export class Jobs { const activeJob = this.store.activeJobForRepo(repo.id) if (activeJob !== undefined) { if (activeJob.kind === 'review') { - const pendingKey = `pending_followup:${repo.id}:${fingerprint}:${input.prUrl}:${input.headSha}`.slice(0, 300) - this.store.setSetting(pendingKey, JSON.stringify(input)) + const pendingKey = pendingFollowUpKey(repo.id, input.prUrl, input.headSha) + this.store.setSetting(pendingKey, mergeParkedFollowUp(this.store.getSetting(pendingKey), input)) return { ok: false, error: '审核结束后会自动跟进', code: 'JOB_ACTIVE' } } return { ok: false, error: '这个仓库已有任务在运行', code: 'JOB_ACTIVE' } @@ -2781,6 +2883,121 @@ export class Jobs { return result } + // #637: a mention is the third way a job starts, beside the scheduler and the + // command surface. The rule lives in `lib/mentionGovernance.ts`; this method is + // only what the rule needs read from the store and what it writes back. + async mention(repo: RepoRow, input: MentionRequest): Promise { + const identity = mentionIdentity(input.target) + const active = this.store.activeJobForRepo(repo.id) + const facts = active === undefined + ? undefined + : { id: active.id, status: active.status as 'queued' | 'running', mention: this.store.getSetting(mentionJobKey(active.id)) ?? null } + const decision = decideMention(input.target, facts) + if (decision.action === 'refuse') { + this.store.audit(repo.user_id, 'trigger.mention_ignored', `${input.commentId} ${identity} ${decision.reason}`) + return { + ok: false, + action: 'refuse', + code: decision.reason === 'draft' ? 'MENTION_DRAFT' : 'MENTION_CLOSED', + error: decision.reason === 'draft' ? '草稿 PR 不会启动任务。' : '已关闭的目标不会启动任务。', + } + } + if (decision.action === 'record') { + // The same target at the same head, while a job holds it: no second job, one + // event on the job that has it. This is the case a reply-to-yourself loop + // and a repeated trigger both land in. + this.store.recordJobEvent({ jobId: decision.jobId, repoId: repo.id, userId: repo.user_id, kind: 'job.mention_recorded', meta: `${input.commentId} ${identity} actor=${input.actor}` }) + this.store.audit(repo.user_id, 'trigger.mention', `${input.commentId} ${identity} actor=${input.actor} outcome=recorded job=${decision.jobId}`) + return { ok: true, action: 'record', jobId: decision.jobId } + } + if (decision.action === 'park') { + // A running job is never interrupted: it finishes, then the newest mention + // is started. The slot holds one mention, replaced, because two mentions on + // the same target are one follow-up — the later one. + this.store.recordJobEvent({ jobId: decision.jobId, repoId: repo.id, userId: repo.user_id, kind: 'job.mention_recorded', meta: `${input.commentId} ${identity} actor=${input.actor}` }) + this.store.setSetting(pendingMentionKey(repo.id), JSON.stringify({ target: input.target, commentId: input.commentId, actor: input.actor })) + this.store.audit(repo.user_id, 'trigger.mention', `${input.commentId} ${identity} actor=${input.actor} outcome=parked job=${decision.jobId}`) + return { ok: true, action: 'park', jobId: decision.jobId } + } + if (decision.action === 'supersede') { + this.supersedeQueuedMention(repo, decision.jobId, input) + const started = await this.startMentionJob(repo, input.target, identity, input.commentId, input.actor) + if (!started.ok || started.action !== 'start') return started + return { ok: true, action: 'supersede', jobId: started.jobId, kind: started.kind } + } + return await this.startMentionJob(repo, input.target, identity, input.commentId, input.actor) + } + + private supersedeQueuedMention(repo: RepoRow, jobId: string, input: MentionRequest): void { + // The queued job has not run yet, so it ends here. It is a status of its own + // rather than a cancellation: nothing failed and nobody stopped it — a newer + // trigger took the slot before it ran, and the row should say so. + this.store.updateJob(jobId, { status: 'superseded', error: '被更新的 mention 取代', error_code: 'MENTION_SUPERSEDED', finished_at: nowIso() }) + this.store.recordJobEvent({ jobId, repoId: repo.id, userId: repo.user_id, kind: 'job.superseded', meta: `mention ${input.commentId}` }) + this.store.audit(repo.user_id, 'trigger.mention_superseded', `${input.commentId} ${jobId} -> ${mentionIdentity(input.target)}`) + } + + private async startMentionJob(repo: RepoRow, target: MentionTarget, identity: string, commentId: string, actor: string): Promise { + const kind = mentionJobKind(target) + const goal = target.kind === 'pull_request' + ? [ + `Review the pull request ${target.url} at ${target.headSha} because a trusted comment mentioned the bot.`, + 'Read the diff and the checks at that head and comment on the pull request with what you find.', + 'Do not write to the branch, do not push, and do not open another pull request.', + ].join('\n') + : [ + `Follow up on the issue ${target.url} because a trusted comment mentioned the bot.`, + 'Open at most one branch and one pull request for this issue, under the one-issue-one-branch-one-PR rule, and report the branch and PR URL.', + 'If no change is warranted, report why and create nothing.', + ].join('\n') + const result = await this.enqueue(repo, 'followup', goal, kind, target.kind === 'pull_request' + ? { branch: target.branch ?? '', prUrl: target.url, ...(target.baseBranch === undefined ? {} : { baseBranch: target.baseBranch }) } + : {}) + if (!result.ok || result.job === undefined) { + this.store.audit(repo.user_id, 'trigger.mention_refused', `${identity} ${result.code ?? 'unknown'}`) + return { ok: false, action: 'start', code: result.code ?? 'MENTION_REFUSED', error: result.error ?? '任务没有启动。' } + } + if (target.kind === 'issue') this.store.updateJob(result.job.id, { issue_url: target.url }) + this.store.setSetting(mentionJobKey(result.job.id), identity) + this.store.audit(repo.user_id, 'trigger.mention', `${commentId} ${identity} actor=${actor} outcome=started job=${result.job.id} kind=${kind}`) + return { ok: true, action: 'start', jobId: result.job.id, kind } + } + + // The parked mention starts when the repository's slot frees, whichever job + // ended. Called from the release pass and when a job finishes. + private async releasePendingMention(repoId: string): Promise { + const key = pendingMentionKey(repoId) + const raw = this.store.getSetting(key) + if (raw === undefined) return + const repo = this.store.getRepo(repoId) + if (repo === undefined) { + this.store.deleteSetting(key) + return + } + // A job still holding the slot keeps the slot; the next pass reads it again. + if (this.store.activeJobForRepo(repoId) !== undefined) return + let parked: { target: MentionTarget; commentId: string; actor: string } + try { + parked = JSON.parse(raw) as typeof parked + } catch { + this.store.deleteSetting(key) + return + } + this.store.deleteSetting(key) + const result = await this.mention(repo, parked) + if (!result.ok) log.warn('pending mention not started', { repo: repoId, code: result.code, error: result.error }) + } + + // Public so the release pass and a test can drive it the same way. + async releaseAllPendingMentions(): Promise { + const repoIds = new Set() + for (const row of this.store.listSettings('pending_mention:')) { + const repoId = row.key.split(':')[1] + if (repoId !== undefined && repoId !== '') repoIds.add(repoId) + } + for (const repoId of repoIds) await this.releasePendingMention(repoId) + } + async notifyOpsAlerts(): Promise { const alerts = this.store.alerts({ usdCny: this.config.quota.usdCny }) const current = new Set() @@ -3244,6 +3461,7 @@ export class Jobs { if (status === 'succeeded' && followUps.length > 0) { await this.queueReviewFollowUps(job, followUps) await this.releasePendingFollowups(job.repo_id) + await this.releasePendingMention(job.repo_id) } if (status === 'succeeded' && job.kind === 'triage' && triage !== null) { // The board is the record, and its first row is what the scheduler can pick @@ -3315,8 +3533,11 @@ export class Jobs { // a review job. ...(follow.instruction === undefined ? {} : { instruction: follow.instruction }), } - const key = `pending_followup:${repo.id}:${job.id}:${index}`.slice(0, 300) - this.store.setSetting(key, JSON.stringify(input)) + // #637: a follow-up the review itself asked for parks in the same slot as + // an update that arrived mid-review, keyed by pull request and head, so the + // release pass queues one job carrying both instead of two jobs. + const key = pendingFollowUpKey(repo.id, input.prUrl, headSha) + this.store.setSetting(key, mergeParkedFollowUp(this.store.getSetting(key), input)) index += 1 } } @@ -3691,6 +3912,7 @@ export class Jobs { const sweptLeases = this.store.sweepScheduleLeases() if (sweptLeases.freed > 0) log.info('schedule leases expired', { freed: sweptLeases.freed }) await this.releaseAllPendingFollowups().catch((error) => log.warn('pending follow-up recovery failed', { error: String(error) })) + await this.releaseAllPendingMentions().catch((error) => log.warn('pending mention recovery failed', { error: String(error) })) await this.releaseDueRetries().catch((error) => log.warn('due retry release failed', { error: String(error) })) await this.reconcileAutoMerge().catch((error) => log.warn('auto-merge reconcile failed', { error: String(error) })) await this.notifyQuestions().catch((error) => log.warn('question notification pass failed', { error: String(error) })) diff --git a/src/lib/issueCommands.ts b/src/lib/issueCommands.ts index fc10b5c7..065d9254 100644 --- a/src/lib/issueCommands.ts +++ b/src/lib/issueCommands.ts @@ -10,12 +10,50 @@ export interface IssueCommandInput { commentId: string } +// #637: a mention is the second way a comment starts work. It is a plain-text +// trigger, not an identity check: the names are configured per deployment, the +// match is case-insensitive at a word boundary, and the trusted-comment set, the +// created-only rule and the comment id are the command path's, not a second set. +export interface MentionTriggerInput { + name: string + issueUrl: string + number: number + title: string + detail: string + association: string + commentId: string + onPullRequest: boolean + actor: string +} + +export interface MentionOptions { + names: readonly string[] + botLogin?: string +} + const ALLOWED_ASSOCIATIONS = new Set(['OWNER', 'MEMBER', 'COLLABORATOR']) // `retry`, `pause`, `resume` and `cancel` act on the job attached to the issue's // work item; the four older verbs create work. Both sets are parsed here so the // webhook has one command vocabulary, not two. const COMMANDS = new Set(['status', 'plan', 'fix', 'approve', 'retry', 'pause', 'resume', 'cancel']) +function objectOf(value: unknown): Record { + return typeof value === 'object' && value !== null ? (value as Record) : {} +} + +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') +} + +// A mention is a whole name after an `@` at a word boundary: `@Fledge` matches, +// `@fledged` and `foo@fledge.example` do not. Matching on the `@` alone would +// make every mail address in a comment a trigger. +function mentionMatcher(names: readonly string[]): RegExp | null { + const usable = names.map((name) => name.trim()).filter((name) => name !== '') + if (usable.length === 0) return null + return new RegExp(`(?:^|[^\\w@])@(${usable.map(escapeRegExp).join('|')})\\b`, 'i') +} + export function parseIssueCommand(body: Record): IssueCommandInput | null { if (body.action !== 'created') return null const comment = typeof body.comment === 'object' && body.comment !== null ? (body.comment as Record) : {} @@ -41,3 +79,40 @@ export function parseIssueCommand(body: Record): IssueCommandIn commentId, } } + +export function parseMentionTrigger(body: Record, options: MentionOptions): MentionTriggerInput | null { + if (body.action !== 'created') return null + const comment = objectOf(body.comment) + const issue = objectOf(body.issue) + const association = typeof comment.author_association === 'string' ? comment.author_association : '' + if (!ALLOWED_ASSOCIATIONS.has(association)) return null + const author = objectOf(comment.user) + const actor = typeof author.login === 'string' ? author.login : '' + // The bot's own comment is never a trigger: an answer that names the word + // would otherwise review its own review, which is the loop this card bounds. + const botLogin = (options.botLogin ?? '').toLowerCase() + if (botLogin !== '' && actor.toLowerCase() === botLogin) return null + const matcher = mentionMatcher(options.names) + if (matcher === null) return null + const text = typeof comment.body === 'string' ? comment.body : '' + const match = matcher.exec(text) + if (match === null) return null + // A closed issue or pull request does not start work: the answer would land on + // a thread nobody is watching, and the trigger would spend budget anyway. + if (issue.state === 'closed') return null + const issueUrl = typeof issue.html_url === 'string' ? issue.html_url : '' + const number = typeof issue.number === 'number' ? issue.number : 0 + const commentId = typeof comment.id === 'number' || typeof comment.id === 'string' ? String(comment.id) : '' + if (issueUrl === '' || number === 0 || commentId === '') return null + return { + name: (match[1] ?? '').toLowerCase(), + issueUrl, + number, + title: typeof issue.title === 'string' ? issue.title.slice(0, 200) : '', + detail: typeof issue.body === 'string' ? issue.body.slice(0, 3000) : '', + association, + commentId, + onPullRequest: typeof issue.pull_request === 'object' && issue.pull_request !== null, + actor, + } +} diff --git a/src/lib/jobFilters.ts b/src/lib/jobFilters.ts index 64069059..6291b4fe 100644 --- a/src/lib/jobFilters.ts +++ b/src/lib/jobFilters.ts @@ -8,7 +8,7 @@ import type { Translate } from '../i18n.js' // same list. Nothing here keeps client state: the route parses the query, the // store filters in SQL, and the page renders the query back into its links. -export const JOB_FILTER_STATUSES: JobStatus[] = ['queued', 'running', 'succeeded', 'failed', 'cancelled', 'paused', 'quota_exhausted'] +export const JOB_FILTER_STATUSES: JobStatus[] = ['queued', 'running', 'succeeded', 'failed', 'cancelled', 'paused', 'quota_exhausted', 'superseded'] // The one list of kinds a job can have is `JobKind` in `src/jobs/runner.ts`; a // second copy here is how `scope` and `verify` went missing from the form while // `research` — a kind no job can have — was offered. `KIND_LABELS` is typed by @@ -64,6 +64,7 @@ const STATUS_LABELS: Record = { cancelled: ['已取消', 'Cancelled'], paused: ['已暂停', 'Paused'], quota_exhausted: ['额度用尽', 'Quota exhausted'], + superseded: ['已被取代', 'Superseded'], } const KIND_LABELS: Record = { diff --git a/src/lib/mentionGovernance.ts b/src/lib/mentionGovernance.ts new file mode 100644 index 00000000..ee2b4dbc --- /dev/null +++ b/src/lib/mentionGovernance.ts @@ -0,0 +1,78 @@ +// #637: a `@fledge` mention is a trigger, and this file is the whole rule for +// what it starts. It is a pure function on purpose: the decision depends on the +// target's state and on the job that is already in flight, and every case is +// cheaper to prove here than through a webhook, a database and a fake GitHub. +// +// The rule (Lead, 2026-11-15): the job kind comes from the mention's target. A +// mention on a pull request starts a review of that pull request at its head +// sha; it reads the diff and comments and never writes to the branch. A mention +// on an issue starts a follow-up for that issue, which may open one branch and +// one pull request under the normal one-issue-one-branch-one-PR rule. +// +// Identity is `(target kind, target number, head sha)`. A mention is the same +// mention while that identity is unchanged, which is what keeps a repeated +// comment from starting a second job: the second one is recorded on the job +// that is already queued or running. +export type MentionTargetKind = 'pull_request' | 'issue' + +export interface MentionTarget { + kind: MentionTargetKind + number: number + url: string + /** Empty for an issue, which has no head; the identity is still stable per issue. */ + headSha: string + state: 'open' | 'closed' + draft: boolean + /** Pull requests only: the head branch the review reads. */ + branch?: string + baseBranch?: string +} + +export interface MentionJobFacts { + id: string + status: 'queued' | 'running' + /** The identity of the mention that started this job; null when it started otherwise. */ + mention: string | null +} + +export type MentionAction = + | { action: 'refuse'; reason: 'closed' | 'draft' } + | { action: 'start' } + | { action: 'record'; jobId: string } + | { action: 'supersede'; jobId: string } + | { action: 'park'; jobId: string } + +/** `(target kind, target number, head sha)` as one comparable string. */ +export function mentionIdentity(target: MentionTarget): string { + return `${target.kind}:${target.number}:${target.headSha}` +} + +/** + * The window a mention can open: a review reads a pull request, a follow-up may + * write to it. The union is `JobKind`-compatible where it is used, so the two + * cannot drift. + */ +export function mentionJobKind(target: MentionTarget): 'review' | 'iterate' { + return target.kind === 'pull_request' ? 'review' : 'iterate' +} + +/** + * What a mention does, given the target and the job already holding the + * repository's one active slot. + * + * - a closed target or a draft pull request starts nothing; + * - with no active job the mention starts one, of the target's kind; + * - the same identity while one is queued or running starts nothing and is + * recorded on that job; + * - a queued job of a different identity is superseded, because it has not + * started work yet; + * - a running job is never interrupted: the mention is parked, and the newest + * one is started when that job ends. + */ +export function decideMention(target: MentionTarget, active: MentionJobFacts | undefined): MentionAction { + if (target.state === 'closed') return { action: 'refuse', reason: 'closed' } + if (target.draft) return { action: 'refuse', reason: 'draft' } + if (active === undefined) return { action: 'start' } + if (active.mention === mentionIdentity(target)) return { action: 'record', jobId: active.id } + return active.status === 'queued' ? { action: 'supersede', jobId: active.id } : { action: 'park', jobId: active.id } +} diff --git a/src/lib/slo.ts b/src/lib/slo.ts index 2279f559..74f15566 100644 --- a/src/lib/slo.ts +++ b/src/lib/slo.ts @@ -27,7 +27,7 @@ export const SLO_JOB_STATUSES = [SLO_SUCCESS_STATUS, 'failed'] as const satisfie // the other half of the definition, exported so the doc that names it // (docs/observability.md) is pinned by the same test as the selectors. `queued` // and `running` are absent from both lists because they are not terminal. -export const EXCLUDED_JOB_STATUSES = ['cancelled', 'paused', 'quota_exhausted'] as const satisfies readonly JobStatus[] +export const EXCLUDED_JOB_STATUSES = ['cancelled', 'paused', 'quota_exhausted', 'superseded'] as const satisfies readonly JobStatus[] // The six queries the report runs, each paired with the field it fills. The // pairing lives here, once, so the CLI cannot label one series with another: it diff --git a/src/views/pages.ts b/src/views/pages.ts index c7ada444..c855958c 100644 --- a/src/views/pages.ts +++ b/src/views/pages.ts @@ -288,6 +288,7 @@ function statusLabel(t: Translate, status: string): string { onboarding: t('待申请', 'Onboarding'), idle: t('空闲', 'Idle'), quota_exhausted: t('额度用完', 'Quota exhausted'), + superseded: t('已被取代', 'Superseded'), paused: t('已暂停', 'Paused'), cancelled: t('已取消', 'Cancelled'), ready: t('就绪', 'Ready'), diff --git a/test/helpers.ts b/test/helpers.ts index 0c72d12d..98d2ece1 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -72,6 +72,7 @@ export function testConfig(overrides: Partial = {}): Config { destructive: { ...base.rateLimit.destructive, ...overrides.rateLimit?.destructive }, expensive: { ...base.rateLimit.expensive, ...overrides.rateLimit?.expensive }, }, + triggers: { ...base.triggers, ...overrides.triggers }, plan: { ...base.plan, ...overrides.plan }, backup: { ...base.backup, ...overrides.backup }, } diff --git a/test/issueCommands.test.ts b/test/issueCommands.test.ts index aafe6b20..61889cb6 100644 --- a/test/issueCommands.test.ts +++ b/test/issueCommands.test.ts @@ -1,6 +1,6 @@ import test from 'node:test' import assert from 'node:assert/strict' -import { parseIssueCommand } from '../src/lib/issueCommands.js' +import { parseIssueCommand, parseMentionTrigger } from '../src/lib/issueCommands.js' const issue = { number: 7, html_url: 'https://github.com/o/r/issues/7', title: 'Fix it', body: 'details' } const base = { action: 'created', issue } @@ -24,3 +24,47 @@ test('issue commands parse only from allowed collaborators and known commands', assert.equal(parseIssueCommand({ ...base, comment: { id: 12, body: '/fledge cancel', author_association: 'OWNER' } })?.command, 'cancel') assert.equal(parseIssueCommand({ ...base, comment: { id: 13, body: '/fledge CANCEL', author_association: 'OWNER' } })?.command, 'cancel') }) + +// #637: a mention is a second matcher in the same parser, not a second parser. +// It uses the same trusted set, the same created-only rule and the same comment +// id, and it recognises a plain-text name at a word boundary. +const prIssue = { ...issue, pull_request: { url: 'https://api.github.com/repos/o/r/pulls/7' } } +const mentionOptions = { names: ['fledge'], botLogin: 'fledge-platform[bot]' } + +test('a trusted mention parses beside /fledge, and only on a created comment', () => { + const parsed = parseMentionTrigger({ action: 'created', issue: prIssue, comment: { id: 21, body: '@fledge have another look', author_association: 'MEMBER', user: { login: 'someone' } } }, mentionOptions) + assert.deepEqual(parsed, { + name: 'fledge', + issueUrl: prIssue.html_url, + number: 7, + title: 'Fix it', + detail: 'details', + association: 'MEMBER', + commentId: '21', + onPullRequest: true, + actor: 'someone', + }) + assert.equal(parseMentionTrigger({ action: 'edited', issue: prIssue, comment: { id: 22, body: '@fledge again', author_association: 'MEMBER' } }, mentionOptions), null) + assert.equal(parseMentionTrigger({ action: 'created', issue: { ...prIssue, state: 'closed' }, comment: { id: 23, body: '@fledge again', author_association: 'MEMBER' } }, mentionOptions), null) +}) + +test('the mention matcher keeps the trusted filter, the bot exclusion and the word boundary', () => { + const mention = (comment: Record, options = mentionOptions) => + parseMentionTrigger({ action: 'created', issue: prIssue, comment: { author_association: 'MEMBER', ...comment } }, options) + // A stranger spends nothing; a collaborator does. + assert.equal(mention({ id: 24, body: '@fledge look' , author_association: 'NONE' }), null) + assert.equal(mention({ id: 25, body: '@fledge look', author_association: 'CONTRIBUTOR' }), null) + assert.ok(mention({ id: 26, body: '@fledge look', author_association: 'COLLABORATOR' })) + // The bot's own comment is not a trigger. + assert.equal(mention({ id: 27, body: '@fledge look', user: { login: 'fledge-platform[bot]' } }), null) + // Case-insensitive, at a word boundary, and only as a whole name. + assert.equal(mention({ id: 28, body: '@Fledge look' })?.name, 'fledge') + assert.equal(mention({ id: 29, body: 'mail foo@fledge.example' }), null) + assert.equal(mention({ id: 30, body: '@fledged look' }), null) + assert.equal(mention({ id: 31, body: 'no mention here' }), null) + // Configured names are the matcher's vocabulary: another deployment's name + // matches only when it is configured, and the earlier name stops matching when + // it is not. + assert.equal(mention({ id: 32, body: '@fledge-bot look' }, { names: ['fledge-bot'], botLogin: 'fledge-platform[bot]' })?.name, 'fledge-bot') + assert.equal(mention({ id: 33, body: '@fledge look' }, { names: ['fledge-bot'], botLogin: 'fledge-platform[bot]' }), null) +}) diff --git a/test/jobs.test.ts b/test/jobs.test.ts index af334b7a..3c77dc3e 100644 --- a/test/jobs.test.ts +++ b/test/jobs.test.ts @@ -590,9 +590,11 @@ test('a follow-up carries the reviewer’s instruction, and two different reques } }) -test('two requests parked behind a review job are both released', async () => { - // The parked key was also built from `reason` alone, so the second request on - // one commit overwrote the first in `pending_followup:` before any job existed. +test('two different updates parked behind a review job become one follow-up carrying both', async () => { + // #637: the parked key carried the request's fingerprint, so two updates that + // arrived before the first model call parked as two rows and later became two + // jobs. One review is one running task: the second update merges into the + // parked request and the release pass queues one job that carries both. const config = testConfig() const store = new Store(':memory:') const runner: Runner = { name: 'capture', async run() { return { status: 'running' } } } @@ -603,36 +605,27 @@ test('two requests parked behind a review job are both released', async () => { const reviewJob = store.createJob({ id: 'job_review_active', repoId: repo.id, userId: user.id, trigger: 'schedule', goal: 'review the PR', callbackHash: 'cb-review', platformTokenHash: '', kind: 'review' }) store.updateJob(reviewJob.id, { status: 'running' }) const prUrl = 'https://github.com/o/p/pull/11' - const first = await jobs.followUp(repo, { prUrl, branch: 'fledge/p', reason: 'review requested changes', headSha: 'fff', instruction: 'First request.' }) - const second = await jobs.followUp(repo, { prUrl, branch: 'fledge/p', reason: 'review requested changes', headSha: 'fff', instruction: 'Second request.' }) + const first = await jobs.followUp(repo, { prUrl, branch: 'fledge/p', reason: 'review requested changes', headSha: 'fff', instruction: 'First request.', trigger: { kind: 'review', commentId: '301' } }) + const second = await jobs.followUp(repo, { prUrl, branch: 'fledge/p', reason: 'mention', headSha: 'fff', instruction: 'Second request.', trigger: { kind: 'mention', commentId: '302', actor: 'alice' } }) assert.equal(first.code, 'JOB_ACTIVE') assert.equal(second.code, 'JOB_ACTIVE') const parked = store.listSettings(`pending_followup:${repo.id}:`) - assert.equal(parked.length, 2, 'two requests park as two entries, not one overwritten') - - // Finishing the review job runs the release pass, which turns a parked - // request into a queued follow-up. One repository may only have one active - // job, so the second request has to stay parked for now — but it must not be - // lost, which is what the overwrite did. The pass walks the settings by key, - // and the key carries the request's fingerprint, so the order is not the - // arrival order: this asserts that neither request is dropped, not which one - // goes first. + assert.equal(parked.length, 1, 'two updates in flight become one parked request') + const merged = JSON.parse(parked[0]?.value ?? '{}') as { requests?: Array<{ instruction?: string; trigger?: { actor?: string; commentId?: string } }> } + assert.deepEqual((merged.requests ?? []).map((request) => request.instruction), ['First request.', 'Second request.'], 'the merged request carries both triggers') + assert.equal(merged.requests?.[1]?.trigger?.actor, 'alice', 'the merged request keeps who sent the second trigger') + + // Finishing the review runs the release pass, which queues ONE follow-up + // carrying both requests: two jobs for one review is the defect. const followUpGoals = (): string[] => store.listJobsPageForUser(user.id, 50, 0) .filter((job) => job.trigger === 'followup') .map((job) => job.goal) store.updateJob(reviewJob.id, { status: 'succeeded' }) await jobs.tick() - assert.equal(followUpGoals().length, 1, 'one parked request is queued') + assert.equal(followUpGoals().length, 1, 'one merged update becomes one job, not two') const queued = followUpGoals()[0] ?? '' - const released = queued.includes('First request.') ? 'First request.' : 'Second request.' - const stillParked = released === 'First request.' ? 'Second request.' : 'First request.' - assert.ok(queued.includes(`Reviewer asked: ${released}`), 'the queued follow-up keeps its own reviewer instruction') - assert.equal(store.listSettings(`pending_followup:${repo.id}:`).length, 1, 'the other request stays parked while the repository has an active job') - const active = store.listJobsPageForUser(user.id, 50, 0).find((job) => job.trigger === 'followup') - assert.ok(active !== undefined) - store.updateJob(active.id, { status: 'failed' }) - await jobs.tick() - assert.ok(followUpGoals().some((goal) => goal.includes(`Reviewer asked: ${stillParked}`)), 'the other request is queued once the first finishes') + assert.match(queued, /First request\./, 'the queued follow-up carries the first request') + assert.match(queued, /Second request\./, 'the queued follow-up carries the second request') assert.equal(store.listSettings(`pending_followup:${repo.id}:`).length, 0, 'nothing stays parked after the release pass') } finally { store.close() diff --git a/test/mentionGovernance.test.ts b/test/mentionGovernance.test.ts new file mode 100644 index 00000000..10731f4e --- /dev/null +++ b/test/mentionGovernance.test.ts @@ -0,0 +1,182 @@ +import test from 'node:test' +import assert from 'node:assert/strict' +import { Store } from '../src/db.js' +import { Jobs } from '../src/jobs/service.js' +import type { Runner } from '../src/jobs/runner.js' +import { decideMention, mentionIdentity, mentionJobKind, type MentionTarget } from '../src/lib/mentionGovernance.js' +import { MemoryMailer, makeUser, testConfig } from './helpers.js' + +// #637: the mention rule is a pure decision, so every branch of it is proven +// here without a webhook, and the three cases that decide the card are proven +// against the service below: a pull-request mention reviews, an issue mention +// follows up, and a second mention at the same head starts nothing. +const PR_URL = 'https://github.com/HugoXK/mention-demo/pull/9' +const ISSUE_URL = 'https://github.com/HugoXK/mention-demo/issues/12' +const HEAD = 'aaaabbbbccccddddeeeeffff0000111122223333' + +const pull: MentionTarget = { + kind: 'pull_request', + number: 9, + url: PR_URL, + headSha: HEAD, + state: 'open', + draft: false, + branch: 'dev/mention', + baseBranch: 'main', +} + +const issue: MentionTarget = { + kind: 'issue', + number: 12, + url: ISSUE_URL, + headSha: '', + state: 'open', + draft: false, +} + +test('#637 identity is the target kind, number and head sha together', () => { + assert.equal(mentionIdentity(pull), `pull_request:9:${HEAD}`) + assert.equal(mentionIdentity(issue), 'issue:12:') + // The same head on another number is another identity, and so is a new head. + assert.notEqual(mentionIdentity(pull), mentionIdentity({ ...pull, number: 10 })) + assert.notEqual(mentionIdentity(pull), mentionIdentity({ ...pull, headSha: 'ffff' })) +}) + +test('#637 the job kind follows the mention target', () => { + assert.equal(mentionJobKind(pull), 'review') + assert.equal(mentionJobKind(issue), 'iterate') +}) + +test('#637 a mention starts, records, supersedes or parks by what already holds the slot', () => { + assert.deepEqual(decideMention(pull, undefined), { action: 'start' }) + // The same identity while the job is queued or running is one mention, twice. + assert.deepEqual(decideMention(pull, { id: 'job_q', status: 'queued', mention: mentionIdentity(pull) }), { action: 'record', jobId: 'job_q' }) + assert.deepEqual(decideMention(pull, { id: 'job_r', status: 'running', mention: mentionIdentity(pull) }), { action: 'record', jobId: 'job_r' }) + // Another identity: a queued job has not run, so a newer mention takes the slot. + assert.deepEqual(decideMention(pull, { id: 'job_q', status: 'queued', mention: 'issue:1:' }), { action: 'supersede', jobId: 'job_q' }) + // A running job is never interrupted; the mention waits, and the newest one runs. + assert.deepEqual(decideMention(pull, { id: 'job_r', status: 'running', mention: 'issue:1:' }), { action: 'park', jobId: 'job_r' }) +}) + +test('#637 a closed target or a draft pull request starts nothing', () => { + assert.deepEqual(decideMention({ ...pull, draft: true }, undefined), { action: 'refuse', reason: 'draft' }) + assert.deepEqual(decideMention({ ...pull, state: 'closed' }, undefined), { action: 'refuse', reason: 'closed' }) + assert.deepEqual(decideMention({ ...issue, state: 'closed' }, undefined), { action: 'refuse', reason: 'closed' }) +}) + +interface Harness { + jobs: Jobs + store: Store + repoId: string + close: () => void +} + +function harness(): Harness { + const config = testConfig() + const store = new Store(':memory:') + const runner: Runner = { name: 'capture', async run() { return { status: 'running' } } } + const jobs = new Jobs(store, config, runner, new MemoryMailer()) + const user = makeUser(store, { login: 'mention-user', status: 'approved' }) + const repo = store.createRepo({ + id: 'repo_mention_governance', + userId: user.id, + owner: 'HugoXK', + name: 'mention-demo', + defaultBranch: 'main', + tokenEnc: 'x', + permission: 'admin', + repoRole: 'maintainer', + goal: '', + autoIterate: false, + intervalMinutes: 1440, + canReview: true, + reviewerEnabled: true, + }) + // A review job has to pass the role-age gate, which a console refresh would do. + store.updateRepo(repo.id, { role_checked_at: new Date().toISOString() }) + return { jobs, store, repoId: repo.id, close: () => store.close() } +} + +test('#637 a pull-request mention starts a review of that pull request', async () => { + const h = harness() + try { + const repo = h.store.getRepo(h.repoId)! + const result = await h.jobs.mention(repo, { target: pull, commentId: '900', actor: 'hugo' }) + assert.equal(result.ok, true) + assert.equal(result.ok ? result.action : '', 'start') + const job = result.ok && result.action === 'start' ? h.store.getJob(result.jobId) : undefined + assert.equal(job?.kind, 'review', 'a mention on a pull request reviews, it does not iterate') + assert.equal(job?.pr_url, PR_URL) + assert.equal(job?.branch, 'dev/mention') + assert.ok(job?.status === 'queued' || job?.status === 'running', 'the mention started the only job the repository may hold') + } finally { + h.close() + } +}) + +test('#637 a mention on an issue starts a follow-up for that issue', async () => { + const h = harness() + try { + const repo = h.store.getRepo(h.repoId)! + const result = await h.jobs.mention(repo, { target: issue, commentId: '901', actor: 'hugo' }) + assert.equal(result.ok, true) + assert.equal(result.ok ? result.action : '', 'start') + const job = result.ok && result.action === 'start' ? h.store.getJob(result.jobId) : undefined + assert.equal(job?.kind, 'iterate', 'a mention on an issue is work on the issue') + assert.equal(job?.issue_url, ISSUE_URL) + assert.equal(job?.pr_url, null, 'it starts no pull request by itself') + } finally { + h.close() + } +}) + +test('#637 a second mention at the same head starts no second job', async () => { + const h = harness() + try { + const repo = h.store.getRepo(h.repoId)! + const first = await h.jobs.mention(repo, { target: pull, commentId: '902', actor: 'hugo' }) + assert.equal(first.ok && first.action, 'start') + const second = await h.jobs.mention(h.store.getRepo(h.repoId)!, { target: pull, commentId: '903', actor: 'hugo' }) + assert.equal(second.ok, true) + assert.equal(second.ok ? second.action : '', 'record') + assert.equal(h.store.listJobsForRepo(h.repoId, 10).length, 1, 'the second mention must not create a job') + const firstJobId = first.ok && first.action === 'start' ? first.jobId : '' + assert.equal(h.store.listJobEvents(firstJobId).some((event) => event.kind === 'job.mention_recorded'), true, 'the mention is recorded on the job that holds it') + // A queued job has not run: hold it in that state, then a new head is a new + // identity and takes the slot instead of being recorded on it. + h.store.updateJob(firstJobId, { status: 'queued' }) + const newer = await h.jobs.mention(h.store.getRepo(h.repoId)!, { target: { ...pull, headSha: 'ffff' }, commentId: '904', actor: 'hugo' }) + assert.equal(newer.ok, true) + assert.equal(newer.ok ? newer.action : '', 'supersede', 'a queued job has not run, so the newer mention takes the slot') + assert.equal(h.store.getJob(firstJobId)?.status, 'superseded') + assert.equal(h.store.listAudit(50).filter((row) => row.action === 'trigger.mention_superseded').length, 1, 'one supersede, one audit row') + } finally { + h.close() + } +}) + +test('#637 a mention that arrives while a job runs waits for it instead of interrupting it', async () => { + const h = harness() + try { + const repo = h.store.getRepo(h.repoId)! + const first = await h.jobs.mention(repo, { target: pull, commentId: '905', actor: 'hugo' }) + assert.equal(first.ok && first.action, 'start') + const firstJobId = first.ok && first.action === 'start' ? first.jobId : '' + h.store.updateJob(firstJobId, { status: 'running', started_at: new Date().toISOString() }) + const second = await h.jobs.mention(h.store.getRepo(h.repoId)!, { target: { ...issue, number: 13, url: 'https://github.com/HugoXK/mention-demo/issues/13' }, commentId: '906', actor: 'hugo' }) + assert.equal(second.ok, true) + assert.equal(second.ok ? second.action : '', 'park') + assert.equal(h.store.getJob(firstJobId)?.status, 'running', 'a running job is never interrupted') + assert.equal(h.store.listJobsForRepo(h.repoId, 10).length, 1, 'nothing starts while the job runs') + // The running job ends, and the newest mention is what runs next. + h.store.updateJob(firstJobId, { status: 'succeeded', finished_at: new Date().toISOString() }) + await h.jobs.releaseAllPendingMentions() + const jobs = h.store.listJobsForRepo(h.repoId, 10) + assert.equal(jobs.length, 2, 'the parked mention starts when the slot frees') + const started = jobs.find((job) => job.id !== firstJobId) + assert.equal(started?.kind, 'iterate') + assert.equal(started?.issue_url, 'https://github.com/HugoXK/mention-demo/issues/13') + } finally { + h.close() + } +}) diff --git a/test/mentionTrigger.test.ts b/test/mentionTrigger.test.ts new file mode 100644 index 00000000..3c685db4 --- /dev/null +++ b/test/mentionTrigger.test.ts @@ -0,0 +1,254 @@ +import test from 'node:test' +import assert from 'node:assert/strict' +import { createHmac } from 'node:crypto' +import { buildApp, type BuiltApp } from '../src/app.js' +import { Store } from '../src/db.js' +import { encryptSecret } from '../src/lib/secrets.js' +import type { Runner } from '../src/jobs/runner.js' +import { json, makeUser, MemoryMailer, readBody, startFakeServer, testConfig } from './helpers.js' + +// #637: a `@fledge` mention in a comment is a trigger, on the same command +// surface as `/fledge` and behind the same trusted-comment filter. These cases +// drive the real webhook route: the matcher, the claim (the comment id is the +// dedupe key), the review it starts and the audit row are exercised together. +const PR_URL = 'https://github.com/HugoXK/mention-demo/pull/9' +const ISSUE_URL = 'https://github.com/HugoXK/mention-demo/issues/12' +const HEAD_SHA = 'aaaabbbbccccddddeeeeffff0000111122223333' +const WEBHOOK_SECRET = 'secret' +const BOT_LOGIN = 'fledge-platform[bot]' + +const runner: Runner = { name: 'capture', async run() { return { status: 'running' } } } + +interface Comment { + path: string + body: string +} + +interface Harness { + built: BuiltApp + store: Store + repoId: string + comments: Comment[] + deliver: (body: string, options?: { association?: string; commentId?: number; login?: string; delivery?: string; draft?: boolean; closed?: boolean; issueTarget?: boolean; headSha?: string }) => Promise + close: () => Promise +} + +async function harness(): Promise { + const comments: Comment[] = [] + let draft = false + const github = await startFakeServer(async (request, response) => { + const raw = await readBody(request) + comments.push({ path: request.url ?? '', body: raw.toString('utf8') }) + const path = (request.url ?? '').split('?')[0] ?? '' + const pull = /\/pulls\/(\d+)$/.exec(path) + if (pull !== null && (request.method ?? 'GET') === 'GET') { + json(response, 200, { + number: Number(pull[1]), + html_url: PR_URL, + title: 'Fix it', + state: 'open', + head: { ref: 'dev/mention', sha: headSha }, + base: { ref: 'main' }, + mergeable: true, + mergeable_state: 'clean', + draft, + merged: false, + changed_files: 1, + body: '', + }) + return + } + json(response, 201, { id: comments.length }) + }) + const config = testConfig({ + github: { ...testConfig().github, apiBase: github.url }, + githubApp: { id: '1', slug: 'fledge-platform', privateKeyPath: '/tmp/fake.pem', webhookSecret: WEBHOOK_SECRET, useUserToken: true }, + }) + const store = new Store(':memory:') + const user = makeUser(store, { login: 'mention-user', status: 'approved' }) + store.setUserGithubToken(user.id, encryptSecret('gh-user-token', config.tokenEncryptionKey), 'repo') + const repo = store.createRepo({ + id: 'repo_mention', + userId: user.id, + owner: 'HugoXK', + name: 'mention-demo', + defaultBranch: 'main', + tokenEnc: '', + permission: 'admin', + repoRole: 'maintainer', + installationId: 42, + goal: '', + autoIterate: false, + intervalMinutes: 1440, + canReview: true, + reviewerEnabled: true, + }) + // A review job has to pass the role-age gate, which a console refresh would do. + store.updateRepo(repo.id, { role_checked_at: new Date().toISOString() }) + const built = await buildApp({ config, store, mailer: new MemoryMailer(), runner, startScheduler: false }) + built.jobs.carryOverReaderOverride = () => ({ read: async () => 'open' }) + + let delivery = 0 + let headSha = HEAD_SHA + const deliver = async (body: string, options: { association?: string; commentId?: number; login?: string; delivery?: string; draft?: boolean; closed?: boolean; issueTarget?: boolean; headSha?: string } = {}): Promise => { + const number = options.issueTarget === true ? 12 : 9 + draft = options.draft === true + headSha = options.headSha ?? HEAD_SHA + const payload = { + action: 'created', + installation: { id: 42 }, + repository: { full_name: 'HugoXK/mention-demo' }, + issue: { + number, + html_url: options.issueTarget === true ? ISSUE_URL : PR_URL, + title: 'Fix it', + body: 'details', + state: options.closed === true ? 'closed' : 'open', + ...(options.issueTarget === true ? {} : { pull_request: { url: `https://api.github.com/repos/HugoXK/mention-demo/pulls/${number}` } }), + }, + comment: { + id: options.commentId ?? 200, + body, + author_association: options.association ?? 'MEMBER', + user: { login: options.login ?? 'issue-command-user', type: options.login === BOT_LOGIN ? 'Bot' : 'User' }, + }, + } + const raw = Buffer.from(JSON.stringify(payload)) + const signature = `sha256=${createHmac('sha256', WEBHOOK_SECRET).update(raw).digest('hex')}` + delivery += 1 + const response = await built.app.inject({ + method: 'POST', + url: '/api/github/webhook', + headers: { + 'content-type': 'application/json', + 'x-github-event': 'issue_comment', + 'x-github-delivery': options.delivery ?? `mention-delivery-${delivery}`, + 'x-hub-signature-256': signature, + }, + payload: raw, + }) + assert.equal(response.statusCode, 200, `webhook delivery failed: ${response.body}`) + return response.statusCode + } + + return { + built, + store, + repoId: repo.id, + comments, + deliver, + close: async () => { + await built.close() + await github.close() + }, + } +} + +function reviewJobs(store: Store, repoId: string) { + return store.listJobsForRepo(repoId, 20).filter((job) => job.pr_url === PR_URL) +} + +function mentionAudits(store: Store) { + return store.listAudit(100).filter((row) => row.action === 'trigger.mention') +} + +test('#637 a collaborator mention on a pull request starts one review job, and the audit row names actor, comment and head', async () => { + const h = await harness() + try { + await h.deliver('@fledge please take another look', { association: 'COLLABORATOR', commentId: 201 }) + const jobs = reviewJobs(h.store, h.repoId) + assert.equal(jobs.length, 1, `mentions should create exactly one review job, got ${jobs.length}`) + assert.equal(jobs[0]?.kind, 'review', 'a mention on a pull request reviews it; it does not write to the branch') + const audits = mentionAudits(h.store) + assert.equal(audits.length, 1, 'one accepted trigger writes one audit row') + const detail = audits[0]?.detail ?? '' + assert.match(detail, /201/, 'the audit row carries the comment id') + assert.match(detail, new RegExp(HEAD_SHA), 'the audit row carries the head SHA the review started on') + assert.match(detail, /issue-command-user/, 'the audit row carries the actor') + assert.ok(h.comments.some((comment) => comment.path.includes('/issues/9/comments')), 'the trigger answers on the pull request') + } finally { + await h.close() + } +}) + +test('#637 a stranger, the bot itself, a draft pull request and a closed one do not trigger work', async () => { + const h = await harness() + try { + await h.deliver('@fledge please review', { association: 'NONE', commentId: 202 }) + await h.deliver('@fledge please review', { login: BOT_LOGIN, commentId: 203 }) + await h.deliver('@fledge please review', { draft: true, commentId: 204 }) + await h.deliver('@fledge please review', { closed: true, commentId: 205 }) + assert.deepEqual(reviewJobs(h.store, h.repoId), [], 'an untrusted, self, draft or closed trigger must not spend model budget') + assert.deepEqual(mentionAudits(h.store), []) + // The control: the same body from a trusted author on an open pull request + // does start a review, so the four refusals above are refusals and not a + // harness that never fires. + await h.deliver('@fledge please review', { association: 'MEMBER', commentId: 209 }) + assert.equal(reviewJobs(h.store, h.repoId).length, 1, 'the harness can start a review, so the four cases above were refused') + } finally { + await h.close() + } +}) + +test('#637 the same comment delivered twice starts one review, and one mention in a body counts once', async () => { + const h = await harness() + try { + await h.deliver('@fledge @fledge please review', { association: 'MEMBER', commentId: 206, delivery: 'dup-a' }) + await h.deliver('@fledge @fledge please review', { association: 'MEMBER', commentId: 206, delivery: 'dup-b' }) + assert.equal(reviewJobs(h.store, h.repoId).length, 1, 'the comment id is the dedupe key: a replayed delivery adds nothing') + } finally { + await h.close() + } +}) + +test('#637 a mention that is not a mention by word boundary is ignored', async () => { + const h = await harness() + try { + await h.deliver('ping me at foo@fledge.example', { association: 'MEMBER', commentId: 207 }) + await h.deliver('@fledged is not a trigger', { association: 'MEMBER', commentId: 208 }) + assert.deepEqual(reviewJobs(h.store, h.repoId), []) + // Same author, same surface, a real mention: one review. So the two bodies + // above were read and refused, not simply unparsed. + await h.deliver('@fledge is a trigger', { association: 'MEMBER', commentId: 210 }) + assert.equal(reviewJobs(h.store, h.repoId).length, 1) + } finally { + await h.close() + } +}) + + +test('#637 a mention on an issue starts a follow-up for that issue, not a review', async () => { + const h = await harness() + try { + await h.deliver('@fledge please take this one', { association: 'MEMBER', commentId: 211, issueTarget: true }) + const jobs = h.store.listJobsForRepo(h.repoId, 20) + assert.equal(jobs.length, 1, 'one mention starts one job') + assert.equal(jobs[0]?.kind, 'iterate', 'an issue mention is work on the issue, which may open a branch and one PR') + assert.equal(jobs[0]?.issue_url, ISSUE_URL, 'the job names the issue it follows up on') + assert.equal(jobs[0]?.pr_url, null, 'a mention does not invent a pull request') + assert.ok(h.comments.some((comment) => comment.path.includes('/issues/12/comments')), 'the trigger answers on the issue') + } finally { + await h.close() + } +}) + +test('#637 a second mention at the same head records on the job instead of starting another', async () => { + const h = await harness() + try { + await h.deliver('@fledge please review', { association: 'MEMBER', commentId: 212 }) + await h.deliver('@fledge please review again', { association: 'MEMBER', commentId: 213 }) + const jobs = h.store.listJobsForRepo(h.repoId, 20) + assert.equal(jobs.length, 1, 'the same target at the same head never opens a second job') + assert.equal(h.store.listJobEvents(jobs[0]!.id).some((event) => event.kind === 'job.mention_recorded'), true, 'the second mention is recorded on the job that has it') + // A new head is a new identity: the queued job is superseded, not appended to. + h.store.updateJob(jobs[0]!.id, { status: 'queued' }) + await h.deliver('@fledge please review the new head', { association: 'MEMBER', commentId: 214, headSha: 'ffff000011112222333344445555666677778888' }) + const after = h.store.listJobsForRepo(h.repoId, 20) + assert.equal(after.length, 2, 'a mention at a new head starts a review of that head') + assert.equal(after.find((job) => job.id !== jobs[0]!.id)?.kind, 'review') + const superseded = h.store.listAudit(100).filter((row) => row.action === 'trigger.mention_superseded') + assert.equal(superseded.length, 1, 'one supersede writes one audit row') + } finally { + await h.close() + } +})