diff --git a/docs/node-maintenance-operator.md b/docs/node-maintenance-operator.md index 957fdc5..f6f9f64 100644 --- a/docs/node-maintenance-operator.md +++ b/docs/node-maintenance-operator.md @@ -10,7 +10,9 @@ reads the canonical PR and displays the selected package versions in the App's and post `/repair selected` in the Conversation tab. The App replies there with the request status and exact Actions link. The native Check action remains an optional secondary entry point. D1 retains the active request and returned Actions run -identity. Dependency repairs and completion handling belong to later slices. +identity. Completed runs update that same reply and release their own request +lock. The current workflow remains a harmless fixture; dependency repairs belong +to later slices. ## Check the selected batch @@ -90,14 +92,21 @@ Comment requests additionally retain the source comment ID, reply ID and exact selections. A unique source-comment index prevents the same comment from authorizing a second attempt, even under a different delivery UUID. Only the winning reservation creates a status reply. Its ID is saved before dispatch; -a lost reply POST or failed save retains the lock and starts no run. Replays -update only the recorded reply owned by this App on this PR. They never retry -an ambiguous reply POST. PR refreshes use read-only PR tokens and do not edit -conversation replies. +a lost reply POST or failed save retains the lock and starts no run. Replays, +losing admissions and PR refreshes do not repaint request output or retry an +ambiguous reply POST. Exposed idle Checks are reused without PATCHing them, +or a fresh idle Check is created. Admission still validates current selections +and revisions; an older idle control cannot authorize a stale batch. The control Check stays completed/neutral while its text changes. Real GitHub did not reopen the previously completed Check when asked for `in_progress`. -The Actions link, and later canonical completion handling, supply run state. +The pending text stays **Request accepted**, with a workflow-page link, until +verified completion. No request-status writes occur after dispatch except +completion updates using the immutable verified outcome. An uncertain pending +write prevents dispatch and retains the reservation for operator recovery. The reply +says **Workflow succeeded**, **Workflow failed** or **Workflow cancelled** and +retains the exact run link and selected versions. The dependency report is not +rewritten: harmless success does not turn “needs review” into “upgraded”. ## Enable comment delivery after review @@ -158,9 +167,31 @@ repeats its dispatch POST. Do not delete an ambiguous row or start another attem until an operator proves no run remains active and reconciles any observed run with the exact request inputs, repository, workflow and actor. -Completion updates and releasing finished requests arrive in slice 4. Until then, -even a finished harmless pilot keeps its active lock. Do not describe its -neutral Check as proof that Actions is still running. +For a known run, redeliver its `workflow_run.completed` event after correcting +an output-update failure. The service fetches the canonical run and checks the +saved installation, repository, run ID, workflow path, dispatch event, original +dispatched branch and App actor. It never searches by run title. An early event +that arrives before the run ID is saved can be redelivered afterward. + +The service saves the verified conclusion and completion time before updating +the recorded App-owned reply and request Check. These fields can be populated +while the state is still active. A transient GitHub failure retains that lock; +redelivery retries safe output updates without dispatching or posting another +reply. Conflicting conclusions require inspection. A missing or replaced old +reply/Check is not overwritten or recreated; the verified run can still finish +its own record. A new PR head's Check is left alone. + +After safe output updates, state becomes `success`, `failure` or `cancelled`. +Other unsuccessful GitHub conclusions map to `failure` and retain the exact +conclusion. The request Check stays neutral and request-bound. A separate idle +Check exposes the current selection action when the same PR head is eligible. +A later deliberate command can start a fresh attempt without manual SQL recovery. +Duplicate completion does not dispatch or create another conversation reply. + +The daily Cron Trigger runs at 03:17 UTC and deletes terminal records older +than 90 days. It never expires active or unknown requests, including those with +a saved outcome awaiting output recovery. Retention bounds replay history; +it does not promise permanent deduplication after a record is deleted. Keep App credentials in operator storage and Worker secrets. Keep verification receipts outside the repository, without credentials or webhook payloads. A diff --git a/docs/node-maintenance.md b/docs/node-maintenance.md index 79f33bc..5b307f6 100644 --- a/docs/node-maintenance.md +++ b/docs/node-maintenance.md @@ -286,13 +286,18 @@ not enable automatic upgrades. Nothing merges automatically. See The repair integration under development keeps selection and submission in the PR's **Conversation** tab. Tick the fixes you allow in the PR description, then post a new comment containing only `/repair selected` using **Comment**. The App -replies with the request status and the exact Actions link. Checkbox edits alone +replies **Request accepted** with a link to the repair workflow page. It keeps +that message until verified completion supplies the final result and exact run +link. It does not show live progress. Checkbox edits alone start no work. The Checks tab is optional for this flow. This requires an installed repair App, a current report that supports comment requests, and the fixed repair workflow. The current pilot only validates inputs; -it changes no dependencies. Completion reporting and the production repair -renderer are still later delivery slices. Nothing merges automatically. +it changes no dependencies. Its existing reply reports success, failure or +cancellation and links to the run. Once completion is safely recorded, a new +command can request another attempt. A failed status update requires operator +recovery before the next attempt. Production repair and report integration +remain later delivery slices. Nothing merges automatically. See the [service reference](node-maintenance-operator.md) for current limits. ## Toolchain selection diff --git a/repair-app/pilot/README.md b/repair-app/pilot/README.md index bca9071..ef4b585 100644 --- a/repair-app/pilot/README.md +++ b/repair-app/pilot/README.md @@ -14,8 +14,17 @@ to fail after writing the receipt. Select `pilot-cancellation` to wait for two minutes so an operator can cancel the run. Use any increasing stable same-minor patch pair. These probes belong only to this installation fixture. -Slice 3 proves action admission, replay rejection and one returned run ID. -Completion and releasing active requests belong to slice 4. Until that slice is -deployed, a finished pilot still retains its active request lock. Inspect its -known Actions run and D1 record; do not remove the lock or dispatch again merely -because the Check still says the workflow started. +Slice 3 proves admission, replay rejection and one returned run ID. Completion +handling updates the existing App reply and request Check from that canonical +run. Success, failure and cancellation release only their own request lock after +safe output updates. The reply explicitly states that this fixture performs no +dependency repair. The package report remains unchanged. + +To verify completion, retain one real signed `workflow_run.completed` delivery +for each outcome, its known run ID, request state and existing reply URL. Redeliver +one completion to prove idempotence. Start a fresh deliberate command after +completion to prove admission without manual SQL recovery. A transient output +failure must retain the active lock and saved conclusion until redelivery. +Restore the original PR description after temporary probe selections, checking +that no concurrent edit would be overwritten. Record actual source/deployment +identities and distinguish local intercepted API tests from real GitHub events. diff --git a/repair-app/runtime-smoke.mjs b/repair-app/runtime-smoke.mjs index 9afcab6..e263170 100644 --- a/repair-app/runtime-smoke.mjs +++ b/repair-app/runtime-smoke.mjs @@ -92,6 +92,8 @@ const commandComment = {id: 501, body: '/repair selected', user: {id: 9, type: ' issue_url: 'https://api.github.com/repos/fixture/repo/issues/12', created_at: '2026-10-24T05:00:00Z', updated_at: '2026-10-24T05:00:00Z'}; let statusComment; +const statusComments = new Map(), idleChecks = [], nativeRuns = new Map(); +let nativeRunID = 900, completionReplyFailure = false; const admissionService = new Miniflare(convertV4MiniflareOptions({ modules: true, scriptPath: new URL('./worker.mjs', import.meta.url).pathname, compatibilityDate: '2026-10-19', d1Databases: ['DB'], @@ -108,32 +110,46 @@ const admissionService = new Miniflare(convertV4MiniflareOptions({ assert.equal(body.permissions.checks, 'write'); return Response.json({token: 'ghs_42.native-action-token'}); } + if (path === '/app') return Response.json({id: 42, slug: 'fixture-maintenance'}); assert.equal(request.headers.get('authorization'), 'Bearer ghs_42.native-action-token'); if (path === '/repos/fixture/repo') return Response.json({id: 7, full_name: 'fixture/repo', default_branch: 'main'}); if (path === '/repos/fixture/repo/pulls/12') return Response.json(nativePR); if (path === '/repos/fixture/repo/collaborators/maintainer/permission') return Response.json({permission: 'maintain', user: {login: 'maintainer'}}); - if (path === '/repos/fixture/repo/issues/comments/501') return Response.json(commandComment); + if (path === `/repos/fixture/repo/issues/comments/${commandComment.id}`) return Response.json(commandComment); if (path === '/repos/fixture/repo/issues/12/comments' && request.method === 'POST') { - statusComment = {id: 600, ...body, performed_via_github_app: {id: 42}, issue_url: commandComment.issue_url}; + statusComment = {id: 600 + statusComments.size, ...body, performed_via_github_app: {id: 42}, issue_url: commandComment.issue_url}; + statusComments.set(statusComment.id, statusComment); return Response.json(statusComment); } - if (path === '/repos/fixture/repo/issues/comments/600') { - if (request.method === 'PATCH') Object.assign(statusComment, body); - return Response.json(statusComment); + const reply = statusComments.get(Number(path.match(/\/issues\/comments\/([0-9]+)$/)?.[1])); + if (reply) { + if (request.method === 'PATCH' && completionReplyFailure) return new Response(null, {status: 503}); + if (request.method === 'PATCH') Object.assign(reply, body); + return Response.json(reply); + } + const check = [nativeCheck, ...idleChecks].find(c => path === `/repos/fixture/repo/check-runs/${c.id}`); + if (check) { + if (request.method === 'PATCH') Object.assign(check, body); + return Response.json(request.method === 'GET' ? documentedCheck(check) : check); } - if (path === '/repos/fixture/repo/check-runs/77') { - if (request.method === 'PATCH') Object.assign(nativeCheck, body); - return Response.json(request.method === 'GET' ? documentedCheck(nativeCheck) : nativeCheck); + if (path === '/repos/fixture/repo/check-runs' && request.method === 'POST') { + const idle = {id: 78 + idleChecks.length, app: {id: 42}, ...body};idleChecks.push(idle);return Response.json(idle); } - if (path.includes('/commits/') && path.endsWith('/check-runs')) return Response.json({total_count: 1, check_runs: [documentedCheck(nativeCheck)]}); + if (path.includes('/commits/') && path.endsWith('/check-runs')) return Response.json({total_count: 1 + idleChecks.length, + check_runs: [nativeCheck, ...idleChecks].map(documentedCheck)}); + if (path.startsWith('/repos/fixture/repo/actions/runs/')) return Response.json(nativeRuns.get(Number(path.split('/').at(-1)))); assert.equal(path, '/repos/fixture/repo/actions/workflows/dependency-repair.yml/dispatches'); assert.equal(request.method, 'POST'); assert.equal(body.ref, 'main'); assert.deepEqual(Object.keys(body.inputs), ['request-id', 'pull-request', 'expected-head', 'expected-base', 'selections']); assert.equal(body.inputs['pull-request'], '12');assert.equal(body.inputs['expected-head'], nativePR.head.sha); assert.equal(body.inputs['expected-base'], nativePR.base.sha);assert.equal(body.inputs.selections, JSON.stringify(nativeItems)); - return Response.json({workflow_run_id: 900, run_url: 'https://api.github.com/repos/fixture/repo/actions/runs/900', - html_url: 'https://github.com/fixture/repo/actions/runs/900'}); + nativeRuns.set(nativeRunID, {id: nativeRunID, repository: {id: 7, full_name: 'fixture/repo'}, + path: '.github/workflows/dependency-repair.yml', event: 'workflow_dispatch', head_branch: 'main', + actor: {type: 'Bot', login: 'fixture-maintenance[bot]'}, status: 'completed', conclusion: 'success', + updated_at: '2026-10-24T10:00:00Z', html_url: `https://github.com/fixture/repo/actions/runs/${nativeRunID}`}); + return Response.json({workflow_run_id: nativeRunID, run_url: `https://api.github.com/repos/fixture/repo/actions/runs/${nativeRunID}`, + html_url: `https://github.com/fixture/repo/actions/runs/${nativeRunID}`}); }, })); try { @@ -220,12 +236,47 @@ try { const commentRows = (await db.prepare('SELECT * FROM requests').all()).results; assert.equal(commentRows.length, 1);assert.equal(commentRows[0].comment_id, 501); assert.equal(commentRows[0].status_comment_id, 600);assert.equal(commentRows[0].run_id, 900); - assert.match(statusComment.body, /Workflow started/);assert.match(statusComment.body, /launch-editor/); - assert.match(statusComment.body, /actions\/runs\/900/); + assert.match(statusComment.body, /Request accepted/);assert.match(statusComment.body, /launch-editor/); + assert.match(statusComment.body, /actions\/workflows\/dependency-repair\.yml/); assert.equal(nativeCalls.filter(c => c.path.endsWith('/dispatches')).length, 3); assert.equal(nativeCalls.filter(c => c.path.endsWith('/12/comments') && c.method === 'POST').length, 1); assert.ok(nativeCalls.every(c => c.body?.status !== 'in_progress')); - console.log('Native conversation command passed: real local D1 deduplicates concurrent/replayed comments, retains one reply, and links the exact dispatched run without requiring Check navigation.'); + console.log('Native conversation command passed: real local D1 deduplicates concurrent/replayed comments, retains one accepted reply with a workflow-page link without requiring Check navigation.'); + + const complete = (runID, installation = 3) => admissionService.dispatchFetch('https://worker.test/webhook', + signed({action: 'completed', installation: {id: installation}, repository: {id: 7, full_name: 'fixture/repo'}, + workflow_run: {id: runID}}, 'workflow_run')); + const callsBeforeUnknown = nativeCalls.length; + assert.equal((await complete(900, 4)).status, 200);assert.equal((await complete(999)).status, 200); + assert.equal(nativeCalls.length, callsBeforeUnknown); + completionReplyFailure = true; + assert.equal((await complete(900)).status, 500); + const pending = await db.prepare('SELECT * FROM requests WHERE id = ?').bind(commandID).first(); + assert.equal(pending.state, 'dispatched');assert.equal(pending.conclusion, 'success');assert.ok(pending.completed_at); + assert.equal((await insert(randomUUID())).meta.changes, 0); + completionReplyFailure = false; + assert.equal((await complete(900)).status, 200); + assert.equal((await db.prepare('SELECT state FROM requests WHERE id = ?').bind(commandID).first()).state, 'success'); + assert.match(statusComments.get(600).body, /Workflow succeeded/); + assert.equal(nativeCheck.output.title, 'Workflow succeeded');assert.equal(idleChecks.length, 1); + assert.equal((await complete(900)).status, 200);assert.equal(idleChecks.length, 1); + + for (const [index, outcome] of ['failure', 'cancelled'].entries()) { + nativeRunID = 901 + index;commandComment.id = 502 + index; + const id = randomUUID();assert.equal((await submit(id)).status, 200); + nativeRuns.get(nativeRunID).conclusion = outcome; + assert.equal((await complete(nativeRunID)).status, 200); + const row = await db.prepare('SELECT * FROM requests WHERE id = ?').bind(id).first(); + assert.equal(row.state, outcome);assert.equal(row.conclusion, outcome); + assert.match(statusComment.body, new RegExp(`Workflow ${outcome === 'failure' ? 'failed' : 'cancelled'}`)); + assert.match(statusComment.body, /pilot performs no dependency repair/); + assert.equal(idleChecks.length, index + 2); + } + assert.equal((await db.prepare("SELECT COUNT(*) AS count FROM requests WHERE state IN ('reserved','dispatched','unknown')").first()).count, 0); + assert.equal(nativeCalls.filter(c => c.path.endsWith('/dispatches')).length, 5); + assert.equal(nativeCalls.filter(c => c.path.endsWith('/12/comments') && c.method === 'POST').length, 3); + assert.match(statusComments.get(600).body, /Workflow succeeded/); + console.log('Native completion passed: actual D1 retains the lock after reply failure; canonical redelivery completes it; isolated success/failure/cancellation update the same replies and admit fresh commands without SQL recovery. GitHub responses are intercepted local fixtures, not live evidence.'); } finally { await admissionService.dispose(); } diff --git a/repair-app/worker.mjs b/repair-app/worker.mjs index 600522d..ac64ba6 100644 --- a/repair-app/worker.mjs +++ b/repair-app/worker.mjs @@ -8,10 +8,11 @@ const VERSION = '(?:0|[1-9][0-9]*)\\.(?:0|[1-9][0-9]*)\\.(?:0|[1-9][0-9]*)'; const PACKAGE = '(?:@[a-z0-9][a-z0-9._-]*/)?[a-z0-9][a-z0-9._-]*'; const LINE = new RegExp('^- \\[([ xX])\\] Allow `(' + PACKAGE + ')` `(' + VERSION + ') → (' + VERSION + ')` by updating its explicit pin\\.$'); const CHECK = 'Dependency repair'; -const MAX_RECONCILIATIONS = 3; const ACTION = {label: 'Run selected fixes', description: 'Run the selected batch in Actions', identifier: 'repair-selected'}; const WORKFLOW = 'dependency-repair.yml'; const ACTIVE = "state IN ('reserved', 'dispatched', 'unknown')"; +const TERMINAL = "state IN ('success', 'failure', 'cancelled')"; +const CONCLUSIONS = new Set(['success', 'failure', 'cancelled', 'timed_out', 'action_required', 'neutral', 'skipped', 'stale', 'startup_failure']); const MAX_INPUT_CHARS = 65_535; const COMMAND = '/repair selected'; const positiveID = value => Number.isSafeInteger(value) && value > 0; @@ -227,29 +228,47 @@ function sameCheckIdentity(left, right) { export function requestStatus(row, items = []) { if (row.selections_json) items = JSON.parse(row.selections_json); - const title = row.state === 'unknown' ? 'Request requires inspection' : row.run_id ? 'Workflow started' : 'Request reserved'; - const summary = row.run_id ? `[View workflow run](${row.html_url}). This pilot performs no dependency repair.` : - 'A request is reserved. Inspect Actions and the request record before recovery; do not retry dispatch automatically.'; - return {title, summary, body: `\n### Dependency repair: ${title}\n\n` + + const outcome = row.conclusion && (row.conclusion === 'success' ? 'succeeded' : row.conclusion === 'cancelled' ? 'cancelled' : 'failed'); + const title = outcome ? `Workflow ${outcome}` : 'Request accepted'; + const summary = outcome ? `[View workflow run](${row.html_url}). This pilot performs no dependency repair.` : + `[View repair workflows](https://github.com/${row.repository}/actions/workflows/${WORKFLOW}).\n\nWaiting for a verified completion result. This pilot performs no dependency repair.`; + const result = outcome ? `${summary}\n\nGitHub conclusion: \`${row.conclusion}\`.` : summary; + return {title, summary: result, body: `\n### Dependency repair: ${title}\n\n` + (items.length ? items.map(item => `- \`${item.name}\` \`${item.from} → ${item.to}\``).join('\n') + '\n\n' : '') + - `${summary}\n\nSelections remain in the PR description. No merge is authorized by this request.`}; + `${result}\n\nSelections remain in the PR description. No merge is authorized by this request.`}; } -async function showRequest(token, repository, row, signal, env, items = [], createComment = false) { +async function requestReply(token, repository, row, env, signal) { + if (!row.comment_id || !row.status_comment_id) return null; + const prefix = `/repos/${repository.full_name}/issues`; + const comment = await api(token, `${prefix}/comments/${row.status_comment_id}`, 'GET', undefined, signal) + .catch(error => { if (row.conclusion && error.status === 404) return null; throw error; }); + if (comment?.id === row.status_comment_id && String(comment.performed_via_github_app?.id) === String(env.APP_ID) && + comment.issue_url === `https://api.github.com${prefix}/${row.pr}` && + comment.body?.startsWith(`\n`)) return comment; + if (!row.conclusion) throw new Error('Status comment identity changed'); + return null; +} + +async function showRequest(token, repository, row, signal, env, items = [], createComment = false, writeCheck = true, reply) { const {title, summary, body} = requestStatus(row, items); - await api(token, `/repos/${repository.full_name}/check-runs/${row.check_id}`, 'PATCH', { + const checkWrite = writeCheck ? api(token, `/repos/${repository.full_name}/check-runs/${row.check_id}`, 'PATCH', { external_id: `node-repair-request:${row.id}`, status: 'completed', conclusion: 'neutral', actions: [], output: {title, summary}, - }, signal); - if (!row.comment_id || !env) return; + }, signal).catch(error => { if (!row.conclusion || error.status !== 404) throw error; }) : Promise.resolve(); + if (!row.comment_id || !env) { await checkWrite; return; } const prefix = `/repos/${repository.full_name}/issues`; if (row.status_comment_id) { - const comment = await api(token, `${prefix}/comments/${row.status_comment_id}`, 'GET', undefined, signal); - if (String(comment.performed_via_github_app?.id) !== String(env.APP_ID) || - comment.issue_url !== `https://api.github.com${prefix}/${row.pr}` || - !comment.body?.startsWith(`\n`)) throw new Error('Status comment identity changed'); - await api(token, `${prefix}/comments/${row.status_comment_id}`, 'PATCH', {body}, signal); - } else if (createComment) { + const writes = await Promise.allSettled([checkWrite, (async () => { + const comment = reply === undefined ? await requestReply(token, repository, row, env, signal) : reply; + if (comment) await api(token, `${prefix}/comments/${row.status_comment_id}`, 'PATCH', {body}, signal) + .catch(error => { if (!row.conclusion || error.status !== 404) throw error; }); + })()]); + const failed = writes.find(result => result.status === 'rejected'); + if (failed) throw failed.reason; + } else { + await checkWrite; + if (!createComment) return; // Only the winning admission creates a reply. An ambiguous POST is never retried. const comment = await api(token, `${prefix}/${row.pr}/comments`, 'POST', {body}, signal); if (!positiveID(comment?.id) || String(comment.performed_via_github_app?.id) !== String(env.APP_ID) || @@ -261,30 +280,26 @@ async function showRequest(token, repository, row, signal, env, items = [], crea } } -async function preserveActiveCheck(token, repository, pr, env, installation, signal) { - const row = await activeRequest(env, installation, repository.id, pr.number); - if (!row || row.head !== pr.head.sha) return row; - await restoreRequestCheck(token, repository, row, env, signal); - return row; +async function retainedCheck(env, installation, repository, check) { + return env.DB ? env.DB.prepare('SELECT * FROM requests WHERE installation_id = ? AND repository_id = ? AND check_id = ? ORDER BY created_at DESC LIMIT 1') + .bind(installation, repository, check).first() : null; } -async function restoreRequestCheck(token, repository, row, env, signal, conversation = false) { - for (let attempt = 0; attempt < MAX_RECONCILIATIONS; attempt++) { - const check = await api(token, `/repos/${repository.full_name}/check-runs/${row.check_id}`, 'GET', undefined, signal); - if (!ownedCheck(check, env.APP_ID, row.head) || - !(check.external_id === `node-repair-request:${row.id}` || new RegExp(`^node-repair:${row.pr}:[a-f0-9]{64}$`).test(check.external_id || ''))) { - throw new Error('Active Check identity changed'); - } - await showRequest(token, repository, row, signal, conversation ? env : undefined); - // Dispatch may advance while either Check call is pending. Repair any stale write. - const current = await env.DB.prepare('SELECT * FROM requests WHERE id = ? AND installation_id = ? AND repository_id = ?') - .bind(row.id, row.installation_id, repository.id).first(); - if (!current || !['reserved', 'dispatched', 'unknown'].includes(current.state)) throw new Error('Active request state changed'); - if (current.state === row.state && current.run_id === row.run_id && current.html_url === row.html_url && - current.status_comment_id === row.status_comment_id) return; - row = current; - } - throw new Error('Active Check state did not settle'); +async function readRequest(env, row) { + return env.DB.prepare('SELECT * FROM requests WHERE id = ? AND installation_id = ? AND repository_id = ?') + .bind(row.id, row.installation_id, row.repository_id).first(); +} + +async function publishCompletion(token, repository, row, env, signal) { + if (!row.conclusion) throw new Error('Completion outcome is not retained'); + const [check, reply] = await Promise.all([ + api(token, `/repos/${repository.full_name}/check-runs/${row.check_id}`, 'GET', undefined, signal) + .catch(error => { if (error.status === 404) return null; throw error; }), + requestReply(token, repository, row, env, signal), + ]); + const writable = check?.id === row.check_id && ownedCheck(check, env.APP_ID, row.head) && + check.external_id === `node-repair-request:${row.id}`; + await showRequest(token, repository, row, signal, env, [], false, writable, reply); } async function requestRepair(token, repository, payload, env, delivery, signal) { @@ -295,12 +310,7 @@ async function requestRepair(token, repository, payload, env, delivery, signal) // A retained delivery is never another dispatch attempt, even after a crash. const replay = await env.DB.prepare('SELECT * FROM requests WHERE installation_id = ? AND repository_id = ? AND (id = ? OR comment_id = ?)') .bind(installation, repository.id, delivery, isComment ? payload.comment.id : null).first(); - if (replay) { - if (['reserved', 'dispatched', 'unknown'].includes(replay.state)) { - await restoreRequestCheck(token, repository, replay, env, signal, isComment); - } - return; - } + if (replay) return; const prefix = `/repos/${repository.full_name}`; const actor = payload.sender?.login; if (payload.sender?.type !== 'User' || typeof actor !== 'string' || !/^[a-z0-9](?:[a-z0-9-]{0,37}[a-z0-9])?$/i.test(actor)) return; @@ -313,10 +323,7 @@ async function requestRepair(token, repository, payload, env, delivery, signal) const submitted = candidate; if (isComment) { if (!submitted) return; - if (submitted.active) { - await restoreRequestCheck(token, repository, submitted.active, env, signal, true); - return; - } + if (submitted.active) return; if (submitted.rejection) { await api(token, `${prefix}/issues/${payload.issue.number}/comments`, 'POST', {body: submitted.rejection}, signal); return; @@ -356,16 +363,11 @@ async function requestRepair(token, repository, payload, env, delivery, signal) comment_id: isComment ? payload.comment.id : null, selections_json: JSON.stringify(items)}; const inserted = await env.DB.prepare(`INSERT OR IGNORE INTO requests (id, installation_id, repository_id, repository, pr, head, base, dispatched_ref, fingerprint, check_id, actor_login, created_at, state, comment_id, selections_json) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`) - .bind(...Object.values(row)).run(); + SELECT ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? + WHERE NOT EXISTS (SELECT 1 FROM requests WHERE installation_id = ? AND repository_id = ? AND check_id = ?)`) + .bind(...Object.values(row), installation, repository.id, current.id).run(); if (inserted.meta?.changes !== 1) { if (inserted.meta?.changes !== 0) throw new Error('Invalid admission result'); - const active = await activeRequest(env, installation, repository.id, number); - if (active?.check_id === current.id) await restoreRequestCheck(token, repository, active, env, signal); - else if (active) await api(token, `${prefix}/check-runs/${current.id}`, 'PATCH', { - actions: [], output: {title: 'Another request is active', summary: - `[Inspect the active Check](https://github.com/${repository.full_name}/runs/${active.check_id}). No new run was started.`}, - }, signal); return; } // Failure here retains reserved state, so redelivery cannot repeat the POST. @@ -382,7 +384,6 @@ async function requestRepair(token, repository, payload, env, delivery, signal) const saved = await env.DB.prepare("UPDATE requests SET state = 'dispatched', run_id = ?, run_url = ?, html_url = ? WHERE id = ? AND installation_id = ? AND repository_id = ? AND state = 'reserved'") .bind(run.workflow_run_id, run.run_url, run.html_url, delivery, installation, repository.id).run(); if (saved.meta?.changes !== 1) throw new Error('Dispatch identity was not retained'); - await showRequest(token, repository, {...row, state: 'dispatched', run_id: run.workflow_run_id, html_url: run.html_url}, signal, env, items); } catch { // The POST may have reached GitHub. Never release this lock or repeat it. await env.DB.prepare("UPDATE requests SET state = 'unknown' WHERE id = ? AND installation_id = ? AND repository_id = ? AND state = 'reserved' AND run_id IS NULL") @@ -422,12 +423,12 @@ async function commentSelection(token, repository, payload, env, signal) { workflowInputs(pr, items); } catch { return rejection; } const fingerprint = await selectionFingerprint(pr, items); - const check = await findSelectionCheck(token, repository, pr, env.APP_ID, `node-repair:${number}:${fingerprint}`, signal); + const check = await findSelectionCheck(token, repository, pr, env.APP_ID, `node-repair:${number}:${fingerprint}`, signal, env, payload.installation.id); if (!check) return rejection; return {check, fingerprint}; } -async function findSelectionCheck(token, repository, pr, appID, external, signal) { +async function findSelectionCheck(token, repository, pr, appID, external, signal, env = {}, installation) { let check, total, read = 0; for (let page = 1; ; page++) { const list = await api(token, `/repos/${repository.full_name}/commits/${pr.head.sha}/check-runs?check_name=Dependency%20repair&app_id=${appID}&filter=all&per_page=100&page=${page}`, 'GET', undefined, signal); @@ -440,6 +441,7 @@ async function findSelectionCheck(token, repository, pr, appID, external, signal if (String(candidate.app?.id) !== String(appID) || candidate.name !== CHECK || candidate.head_sha !== pr.head.sha || candidate.external_id !== external) continue; if (!positiveID(candidate.id) || candidate.status !== 'completed') throw new Error('Repair Check is not idle'); + if (await retainedCheck(env, installation, repository.id, candidate.id)) continue; if (!check || candidate.id > check.id) check = candidate; } read += list.check_runs.length; @@ -447,46 +449,86 @@ async function findSelectionCheck(token, repository, pr, appID, external, signal } } -async function refreshSelectionCheck(token, repository, number, appID, signal, env = {}, installation) { +async function refreshSelectionCheck(token, repository, number, appID, signal, env = {}, installation, expectedHead) { const prefix = `/repos/${repository.full_name}`; - for (let attempt = 0; attempt < MAX_RECONCILIATIONS; attempt++) { - const pr = await api(token, `${prefix}/pulls/${number}`, 'GET', undefined, signal); - if (!managedPR(pr, repository, number)) return; - const active = await preserveActiveCheck(token, repository, pr, env, installation, signal); - if (active?.head === pr.head.sha) return; - let items = [], valid = true; - try { items = selections(pr.body); workflowInputs(pr, items); } catch { valid = false; items = []; } - const external = `node-repair:${number}:${await selectionFingerprint(pr, items)}`; - const state = `${valid ? 'valid' : 'invalid'}:${external}`; - const check = await findSelectionCheck(token, repository, pr, appID, external, signal); - const output = { - title: !valid ? 'Repair selections need attention' : items.length ? `${items.length} selected fix${items.length === 1 ? '' : 'es'}` : 'No fixes selected', - summary: !valid ? 'Use one valid selection block in the PR description. No run was started.' : items.length ? - items.map(item => `- \`${item.name}\` \`${item.from} → ${item.to}\``).join('\n') + - (active ? '\n\nAnother request is active. Inspect it before requesting a new attempt.' : env.DB ? - '\n\nPost `/repair selected` in the PR conversation to request these fixes. The native Run selected fixes action is also available here. The pilot only validates inputs.' : - '\n\nSelections are displayed for review. Running fixes is not enabled yet.') : - 'Select the allowed fixes in the PR description. Editing checkboxes does not start a run.', - }; - await api(token, `${prefix}/check-runs${check ? `/${check.id}` : ''}`, check ? 'PATCH' : 'POST', { - ...(check ? {} : {name: CHECK, head_sha: pr.head.sha}), external_id: external, - status: 'completed', conclusion: 'neutral', output, - actions: valid && items.length && env.DB && !active ? [ACTION] : [], - }, signal); - // Repair a concurrent idle write that raced an action's reservation/Check update. - const reserved = await preserveActiveCheck(token, repository, pr, env, installation, signal); - if (reserved?.head === pr.head.sha) return; - const current = await api(token, `${prefix}/pulls/${number}`, 'GET', undefined, signal); - if (!managedPR(current, repository, number)) return; - let currentItems = [], currentValid = true; - try { currentItems = selections(current.body); workflowInputs(current, currentItems); } catch { currentValid = false; currentItems = []; } - const currentExternal = `node-repair:${number}:${await selectionFingerprint(current, currentItems)}`; - if (`${currentValid ? 'valid' : 'invalid'}:${currentExternal}` === state) return; + const pr = await api(token, `${prefix}/pulls/${number}`, 'GET', undefined, signal); + if (!managedPR(pr, repository, number) || (expectedHead && pr.head.sha !== expectedHead)) return; + const active = await activeRequest(env, installation, repository.id, number); + if (active?.head === pr.head.sha) return; + let items = [], valid = true; + try { items = selections(pr.body); workflowInputs(pr, items); } catch { valid = false; items = []; } + const external = `node-repair:${number}:${await selectionFingerprint(pr, items)}`; + // An exposed control may already be admitted when this read returns. Never PATCH it. + if (await findSelectionCheck(token, repository, pr, appID, external, signal, env, installation)) return; + const output = { + title: !valid ? 'Repair selections need attention' : items.length ? `${items.length} selected fix${items.length === 1 ? '' : 'es'}` : 'No fixes selected', + summary: !valid ? 'Use one valid selection block in the PR description. No run was started.' : items.length ? + items.map(item => `- \`${item.name}\` \`${item.from} → ${item.to}\``).join('\n') + + (env.DB ? '\n\nPost `/repair selected` in the PR conversation to request these fixes. Only one request can be active per PR. The native Run selected fixes action is also available here. The pilot only validates inputs.' : + '\n\nSelections are displayed for review. Running fixes is not enabled yet.') : + 'Select the allowed fixes in the PR description. Editing checkboxes does not start a run.', + }; + await api(token, `${prefix}/check-runs`, 'POST', { + name: CHECK, head_sha: pr.head.sha, external_id: external, + status: 'completed', conclusion: 'neutral', output, + actions: valid && items.length && env.DB ? [ACTION] : [], + }, signal); +} + +async function completeRequest(payload, env, signal) { + const installation = payload.installation?.id, repository = payload.repository, runID = payload.workflow_run?.id; + if (!env.DB || ![installation, repository?.id, runID].every(positiveID)) return; + const found = await env.DB.prepare('SELECT * FROM requests WHERE installation_id = ? AND repository_id = ? AND run_id = ? LIMIT 2') + .bind(installation, repository.id, runID).all(); + if (!Array.isArray(found.results) || found.results.length > 1) throw new Error('Ambiguous run identity'); + let row = found.results[0]; + if (!row || repository.full_name?.toLowerCase() !== row.repository.toLowerCase()) return; + const prefix = `/repos/${row.repository}`; + const [token, app] = await Promise.all([ + installationToken(env, installation, repository.id, signal, {contents: 'read', + pull_requests: row.comment_id ? 'write' : 'read', checks: 'write', actions: 'read', + ...(row.comment_id ? {issues: 'read'} : {})}), + appJWT(env).then(jwt => api(jwt, '/app', 'GET', undefined, signal)), + ]); + const [run, currentRepository] = await Promise.all([ + api(token, `${prefix}/actions/runs/${runID}`, 'GET', undefined, signal), + api(token, prefix, 'GET', undefined, signal), + ]); + if (String(app?.id) !== String(env.APP_ID) || typeof app.slug !== 'string' || + run?.id !== runID || run.repository?.id !== row.repository_id || + run.repository.full_name?.toLowerCase() !== row.repository.toLowerCase() || + currentRepository?.id !== row.repository_id || currentRepository.full_name?.toLowerCase() !== row.repository.toLowerCase() || + run.path !== `.github/workflows/${WORKFLOW}` || run.event !== 'workflow_dispatch' || + run.head_branch !== row.dispatched_ref || run.actor?.type !== 'Bot' || run.actor.login !== `${app.slug}[bot]` || + run.html_url !== row.html_url || run.status !== 'completed') return; + const completed = Date.parse(run.updated_at); + if (!CONCLUSIONS.has(run.conclusion) || typeof run.updated_at !== 'string' || !Number.isFinite(completed) || completed <= 0) throw new Error('Invalid completed run'); + if (row.conclusion && row.conclusion !== run.conclusion) throw new Error('Conflicting retained outcome'); + if (!row.conclusion) { + // Keep the active unique-index lock until known outputs are safely updated. + await env.DB.prepare(`UPDATE requests SET conclusion = ?, completed_at = ? + WHERE id = ? AND installation_id = ? AND repository_id = ? AND run_id = ? AND conclusion IS NULL AND ${ACTIVE}`) + .bind(run.conclusion, completed, row.id, installation, repository.id, runID).run(); } - throw new Error('Repair Check freshness did not settle'); + row = await readRequest(env, row); + if (row?.conclusion !== run.conclusion || !Number.isFinite(row.completed_at) || row.completed_at <= 0) throw new Error('Outcome was not retained'); + await publishCompletion(token, currentRepository, row, env, signal); + const state = ['success', 'cancelled'].includes(run.conclusion) ? run.conclusion : 'failure'; + await env.DB.prepare(`UPDATE requests SET state = ? WHERE id = ? AND installation_id = ? AND repository_id = ? + AND run_id = ? AND conclusion = ? AND ${ACTIVE}`) + .bind(state, row.id, installation, repository.id, runID, run.conclusion).run(); + const terminal = await readRequest(env, row); + if (terminal?.state !== state || terminal.conclusion !== run.conclusion) throw new Error('Terminal state was not retained'); + // A terminal request keeps its own Check. A fresh idle control is separate. + await refreshSelectionCheck(token, currentRepository, row.pr, env.APP_ID, signal, env, installation, row.head); } export default { + async scheduled(event, env) { + if (!env.DB || !Number.isFinite(event.scheduledTime)) throw new Error('Retention is not configured'); + await env.DB.prepare(`DELETE FROM requests WHERE ${TERMINAL} AND completed_at < ?`) + .bind(event.scheduledTime - 90 * 86_400_000).run(); + }, async fetch(request, env) { if (request.method !== 'POST' || new URL(request.url).pathname !== '/webhook') return new Response('Not found', {status: 404}); if (!['WEBHOOK_SECRET', 'APP_PRIVATE_KEY'].every(name => typeof env[name] === 'string' && env[name]) || @@ -510,6 +552,8 @@ export default { await readRepository(payload, env, controller.signal, request.headers.get('x-github-delivery')); } else if (event === 'issue_comment' && repairComment(payload)) { await readRepository(payload, env, controller.signal, request.headers.get('x-github-delivery')); + } else if (event === 'workflow_run' && payload.action === 'completed') { + await completeRequest(payload, env, controller.signal); } controller.signal.throwIfAborted(); return new Response('Accepted', {status: 200}); diff --git a/repair-app/worker.test.mjs b/repair-app/worker.test.mjs index 481e0c7..83a2ea7 100644 --- a/repair-app/worker.test.mjs +++ b/repair-app/worker.test.mjs @@ -320,7 +320,7 @@ function checkMock({changePR, list = {total_count: 0, check_runs: []}, failPath} return {calls, pr, repository}; } const ownCheck = (overrides = {}) => ({id: 77, app: {id: 42}, name: 'Dependency repair', - head_sha: 'a'.repeat(40), external_id: 'node-repair:12:33b17f82caff79dd2065972e92b5e5b83aa761be9c5799dcbbced028f0f60849', status: 'completed', ...overrides}); + head_sha: 'a'.repeat(40), external_id: 'node-repair:12:33b17f82caff79dd2065972e92b5e5b83aa761be9c5799dcbbced028f0f60849', status: 'completed', conclusion: 'neutral', ...overrides}); const listOf = (...checks) => ({total_count: checks.length, check_runs: checks}); const checkWrites = calls => calls.filter(call => call.path.includes('/check-runs') && call.method !== 'GET'); const managedPR = (body, headSha = 'a'.repeat(40), state = 'open') => ({number: 12, state, user: {type: 'Bot'}, body, @@ -340,24 +340,20 @@ test('canonical bot PR creates an App Check with exact selections and no action external_id: `node-repair:12:${await selectionFingerprint(pr, selections(pr.body))}`, status: 'completed', conclusion: 'neutral', output: {title: '1 selected fix', summary: '- `launch-editor` `2.14.1 → 2.14.2`\n\nSelections are displayed for review. Running fixes is not enabled yet.'}, actions: []}); - assert.equal(calls.length, 6); + assert.equal(calls.length, 5); }); for (const action of ['opened', 'edited', 'synchronize', 'reopened']) { - test(`managed PR ${action} refreshes its own idle Check`, async () => { + test(`managed PR ${action} reuses an exposed idle Check without writing`, async () => { const {calls} = checkMock({list: listOf(ownCheck())}); assert.equal((await worker.fetch(signed({...payload, action, number: 12}, 'pull_request'), env)).status, 200); - const writes = checkWrites(calls); assert.equal(writes.length, 1); - assert.equal(writes[0].path, '/repos/fixture/repo/check-runs/77'); - assert.equal(writes[0].method, 'PATCH'); - assert.deepEqual(writes[0].body.actions, []); - assert.equal(writes[0].body.head_sha, undefined); + assert.equal(checkWrites(calls).length, 0); }); } test('selection edits and clearing the block replace the idle fingerprint and summary', async () => { const versions = [], identities = []; - for (const body of [fixtures.cases[0].body, fixtures.cases[0].body.replace('2.14.2', '2.14.3'), + for (const body of [fixtures.cases[0].body.replace('2.14.2', '2.14.3'), fixtures.cases[1].body, 'malformed selection']) { const {calls} = checkMock({changePR: pr => { pr.body = body; }, list: listOf(ownCheck())}); assert.equal((await worker.fetch(signed({...payload, number: 12}, 'pull_request'), env)).status, 200); @@ -365,9 +361,9 @@ test('selection edits and clearing the block replace the idle fingerprint and su identities.push(output.external_id);versions.push(output.output.summary); assert.deepEqual(output.actions, []); } - assert.notEqual(identities[0], identities[1]);assert.notEqual(identities[1], identities[2]); - assert.match(versions[1], /2\.14\.3/);assert.doesNotMatch(versions[2], /launch-editor/); - assert.match(versions[3], /valid selection block/); + assert.notEqual(identities[0], identities[1]);assert.equal(identities[1], identities[2]); + assert.match(versions[0], /2\.14\.3/);assert.doesNotMatch(versions[1], /launch-editor/); + assert.match(versions[2], /valid selection block/); }); for (const [name, body] of [['changed', fixtures.cases[0].body.replace('2.14.2', '2.14.3')], @@ -435,14 +431,10 @@ for (const list of [{}, {total_count: 101, check_runs: []}, {total_count: -1, ch }); } -test('duplicate owned idle Checks refresh the newest Check', async () => { +test('duplicate owned idle Checks are reused without PATCHing either', async () => { const {calls} = checkMock({list: listOf(ownCheck({id: 78}), ownCheck({id: 77}))}); assert.equal((await worker.fetch(signed({...payload, number: 12}, 'pull_request'), env)).status, 200); - const writes = checkWrites(calls); - assert.equal(writes.length, 1); - assert.equal(writes[0].path, '/repos/fixture/repo/check-runs/78'); - assert.equal(writes[0].method, 'PATCH'); - assert.deepEqual(writes[0].body.actions, []); + assert.equal(checkWrites(calls).length, 0); }); test('overlapping initial deliveries recover from duplicate idle Checks on a later edit', async () => { @@ -499,7 +491,7 @@ test('overlapping initial deliveries recover from duplicate idle Checks on a lat assert.deepEqual(writes[2].body.actions, []); }); -test('overlapping edits reconcile an existing Check to the current canonical selection', async () => { +test('overlapping edits never PATCH the exposed older control', async () => { const repository = {id: 7, full_name: 'fixture/repo', default_branch: 'main'}; let body = fixtures.cases[0].body; const calls = [], writes = []; @@ -540,107 +532,16 @@ test('overlapping edits reconcile an existing Check to the current canonical sel const olderResponse = await older; assert.equal(newerResponse.status, 200); assert.equal(olderResponse.status, 200); - assert.equal(writes.length, 3); + assert.equal(writes.length, 1); assert.match(writes[0].output.summary, /2\.14\.3/); - assert.match(writes[1].output.summary, /2\.14\.2/); - assert.match(writes[2].output.summary, /2\.14\.3/); - assert.equal(writes[0].external_id, writes[2].external_id); - assert.notEqual(writes[1].external_id, writes[2].external_id); + assert.equal(calls.filter(call => call.method === 'PATCH').length, 0); assert.ok(writes.every(write => write.actions.length === 0)); }); -test('repeated canonical changes exhaust bounded Check reconciliation', async () => { - const repository = {id: 7, full_name: 'fixture/repo', default_branch: 'main'}; - const writes = []; - let prReads = 0, currentExternal; - fetch.mock.mockImplementation(async (url, options) => { - const path = new URL(url).pathname; - const requestBody = options.body === undefined ? undefined : JSON.parse(options.body); - assert.equal(options.redirect, 'manual'); - if (path === '/app/installations/3/access_tokens') return json({token}); - assert.equal(options.headers.Authorization, `Bearer ${token}`); - if (path === '/repos/fixture/repo') return json(repository); - if (path === '/repos/fixture/repo/pulls/12') { - prReads++; - const current = managedPR(fixtures.cases[0].body.replace('2.14.2', `2.14.${prReads + 1}`)); - currentExternal = `node-repair:12:${await selectionFingerprint(current, selections(current.body))}`; - return json(current); - } - if (path === `/repos/fixture/repo/commits/${'a'.repeat(40)}/check-runs`) - return json(listOf(ownCheck({external_id: currentExternal}))); - if (path === '/repos/fixture/repo/check-runs/77' && options.method === 'PATCH') { - writes.push(requestBody); - return json({id: 77}); - } - throw new Error(`Unexpected API path: ${options.method} ${path}`); - }); - const response = await worker.fetch(signed({...payload, number: 12, action: 'edited'}, 'pull_request'), env); - assert.equal(response.status, 500); - assert.equal(prReads, 6); - assert.equal(writes.length, 3); - assert.deepEqual(writes.map(write => write.actions), [[], [], []]); -}); - -test('eligibility loss after a Check write stops reconciliation safely', async () => { - const repository = {id: 7, full_name: 'fixture/repo', default_branch: 'main'}; - let prReads = 0; - const writes = []; - fetch.mock.mockImplementation(async (url, options) => { - const path = new URL(url).pathname; - const requestBody = options.body === undefined ? undefined : JSON.parse(options.body); - assert.equal(options.redirect, 'manual'); - if (path === '/app/installations/3/access_tokens') return json({token}); - assert.equal(options.headers.Authorization, `Bearer ${token}`); - if (path === '/repos/fixture/repo') return json(repository); - if (path === '/repos/fixture/repo/pulls/12') { - prReads++; - return json(managedPR(fixtures.cases[0].body, 'a'.repeat(40), prReads === 2 ? 'closed' : 'open')); - } - if (path === `/repos/fixture/repo/commits/${'a'.repeat(40)}/check-runs`) return json(listOf(ownCheck())); - if (path === '/repos/fixture/repo/check-runs/77' && options.method === 'PATCH') { - writes.push(requestBody); - return json({id: 77}); - } - throw new Error(`Unexpected API path: ${options.method} ${path}`); - }); - const response = await worker.fetch(signed({...payload, number: 12, action: 'edited'}, 'pull_request'), env); - assert.equal(response.status, 200); - assert.equal(prReads, 2); - assert.equal(writes.length, 1); - assert.deepEqual(writes[0].actions, []); -}); - -test('head changes during reconciliation refresh the new canonical head', async () => { - const repository = {id: 7, full_name: 'fixture/repo', default_branch: 'main'}; - const oldHead = 'a'.repeat(40), newHead = 'c'.repeat(40); - let prReads = 0; - const calls = []; - fetch.mock.mockImplementation(async (url, options) => { - const path = new URL(url).pathname; - const requestBody = options.body === undefined ? undefined : JSON.parse(options.body); - calls.push({path, method: options.method, body: requestBody}); - assert.equal(options.redirect, 'manual'); - if (path === '/app/installations/3/access_tokens') return json({token}); - assert.equal(options.headers.Authorization, `Bearer ${token}`); - if (path === '/repos/fixture/repo') return json(repository); - if (path === '/repos/fixture/repo/pulls/12') { - prReads++; - return json(managedPR(fixtures.cases[0].body, prReads === 1 ? oldHead : newHead)); - } - if (path === `/repos/fixture/repo/commits/${oldHead}/check-runs`) return json(listOf(ownCheck())); - if (path === `/repos/fixture/repo/commits/${newHead}/check-runs`) return json(listOf()); - if (path === '/repos/fixture/repo/check-runs/77' && options.method === 'PATCH') return json({id: 77}); - if (path === '/repos/fixture/repo/check-runs' && options.method === 'POST') return json({id: 88}); - throw new Error(`Unexpected API path: ${options.method} ${path}`); - }); - const response = await worker.fetch(signed({...payload, number: 12, action: 'edited'}, 'pull_request'), env); - assert.equal(response.status, 200); - const writes = checkWrites(calls); - assert.equal(writes.length, 2); - assert.equal(writes[0].method, 'PATCH'); - assert.equal(writes[1].method, 'POST'); - assert.equal(writes[1].body.head_sha, newHead); - assert.deepEqual(writes.map(write => write.body.actions), [[], []]); +test('canonical refresh does not mutate an exposed matching control', async () => { + const {calls} = checkMock({list: listOf(ownCheck())}); + assert.equal((await worker.fetch(signed({...payload, number: 12, action: 'edited'}, 'pull_request'), env)).status, 200); + assert.equal(checkWrites(calls).length, 0); }); test('canonical PR or Check-list API failures are sanitized and cannot cause writes', async () => { @@ -661,6 +562,7 @@ function requestDatabase() { return {sql, prepare(query) { return {bind(...values) { return {async first() { return sql.prepare(query).get(...values) ?? null; }, + async all() { return {results: sql.prepare(query).all(...values)}; }, async run() { return {meta: {changes: Number(sql.prepare(query).run(...values).changes)}}; }}; }}; }}; @@ -724,11 +626,11 @@ test('an authorized current action reserves once and retains the documented run assert.equal(row.installation_id, 3);assert.equal(row.repository_id, 7);assert.equal(row.dispatched_ref, 'main'); assert.equal(row.completed_at, null);assert.equal(row.conclusion, null); assert.equal(f.check.external_id, `node-repair-request:${delivery}`);assert.equal(f.check.status, 'completed');assert.equal(f.check.conclusion, 'neutral'); - assert.deepEqual(f.check.actions, []);assert.match(f.check.output.summary, /actions\/runs\/900/); + assert.deepEqual(f.check.actions, []);assert.match(f.check.output.summary, /actions\/workflows\/dependency-repair\.yml/); const before = f.calls.length; assert.equal((await f.click()).status, 200); assert.equal(f.dispatches().length, 1);assert.equal(f.rows().length, 1); - assert.equal(f.calls.length - before, 4); // Token, repository and owned Check reconciliation; no new attempt. + assert.equal(f.calls.length - before, 2); // Token and repository only; replay never repaints output. }); async function commentMock(t, options = {}) { @@ -761,15 +663,365 @@ async function commentMock(t, options = {}) { return {...f, command, comments, commentEvent: event, submit}; } +async function completionMock(t, {native = false, admit = true, onCall} = {}) { + const app = {id: 42, slug: 'fixture-maintenance'}; + const run = {id: 900, repository: {id: 7, full_name: 'fixture/repo'}, + path: '.github/workflows/dependency-repair.yml', event: 'workflow_dispatch', head_branch: 'main', + actor: {type: 'Bot', login: 'fixture-maintenance[bot]'}, status: 'completed', conclusion: 'success', + updated_at: '2026-10-24T10:00:00Z', html_url: 'https://github.com/fixture/repo/actions/runs/900'}; + const idle = []; + const f = await (native ? actionMock : commentMock)(t, {async onCall(call, context) { + const intercepted = await onCall?.(call, {...context, app, run, idle}); + if (intercepted) return intercepted; + if (call.path === '/app') { + assert.equal(call.method, 'GET'); + return json(app); + } + if (call.path === '/repos/fixture/repo/actions/runs/900') return json(run); + if (call.path.includes('/commits/') && call.path.endsWith('/check-runs')) return json(listOf(context.check, ...idle)); + if (call.path === '/repos/fixture/repo/check-runs' && call.method === 'POST') { + const check = {id: 78 + idle.length, app: {id: 42}, ...call.body}; + idle.push(check);return json(check); + } + const check = idle.find(c => call.path === `/repos/fixture/repo/check-runs/${c.id}`); + if (check) { + if (call.method === 'PATCH') Object.assign(check, call.body); + return json(check); + } + }}); + if (admit) assert.equal((await (native ? f.click() : f.submit())).status, 200); + const event = {...payload, action: 'completed', workflow_run: {id: 900}}; + const complete = (changes = {}) => worker.fetch(signed({...event, ...changes}, 'workflow_run'), f.environment); + return {...f, app, run, idle, complete}; +} + +function pauseNextAdmission(db) { + const reached = Promise.withResolvers(); + const release = Promise.withResolvers(); + const originalPrepare = db.prepare.bind(db); + let armed = true; + db.prepare = query => { + const statement = originalPrepare(query); + if (!armed || !query.includes('INSERT OR IGNORE INTO requests')) return statement; + armed = false; + return {bind(...values) { + const bound = statement.bind(...values); + return {run: async () => { + reached.resolve(); + await release.promise; + return bound.run(); + }}; + }}; + }; + return {reached: reached.promise, release: release.resolve}; +} + +function commentSubmission(f, deliveryID, commentID) { + const event = structuredClone(f.commentEvent); + event.comment.id = commentID; + event.comment.user.id = event.sender.id = 9; + return worker.fetch(signed(event, 'issue_comment', {'x-github-delivery': deliveryID}), f.environment); +} + +for (const [name, start] of [ + ['native/native', (f, firstID, secondID) => Promise.all([f.click(firstID), f.click(secondID)])], + ['comment/comment', (f, firstID, secondID) => Promise.all([ + commentSubmission(f, firstID, 501), commentSubmission(f, secondID, 502), + ])], + ['native/comment', (f, firstID, secondID) => Promise.all([ + f.click(firstID), commentSubmission(f, secondID, 502), + ])], +]) { + test(`a delayed ${name} loser cannot repaint a terminal Check after a third admission`, {timeout: 3_000}, async t => { + const sourceComments = new Map(); + const f = await completionMock(t, {admit: false, native: name === 'native/native', onCall(call, context) { + const match = /\/issues\/comments\/([0-9]+)$/.exec(call.path); + if (call.method === 'GET' && match && ['501', '502'].includes(match[1])) { + const id = Number(match[1]); + if (!sourceComments.has(id)) sourceComments.set(id, {...context.command, id}); + return json(sourceComments.get(id)); + } + }}); + const barrier = pauseNextAdmission(f.db);t.after(barrier.release); + const firstID = '94e535a5-2a7d-491d-858a-3df51ce211de'; + const secondID = 'a4e535a5-2a7d-491d-858a-3df51ce211df'; + const submissions = start(f, firstID, secondID); + await barrier.reached; + for (let attempt = 0; attempt < 100 && f.rows().length === 0; attempt++) { + await new Promise(resolve => setImmediate(resolve)); + } + assert.equal(f.rows().length, 1); + assert.equal((await f.complete()).status, 200); + assert.equal((await f.click('b4e535a5-2a7d-491d-858a-3df51ce211df', {check_run: f.idle[0]})).status, 200); + barrier.release(); + assert.deepEqual((await submissions).map(response => response.status), [200, 200]); + assert.equal(f.rows().length, 2); + assert.equal(f.rows()[1].state, 'dispatched'); + assert.equal(f.rows()[0].state, 'success'); + assert.equal(f.rows()[0].check_id, 77); + assert.equal(f.dispatches().length, 2); + assert.equal(f.check.external_id, `node-repair-request:${f.rows()[0].id}`); + assert.equal(f.check.output.title, 'Workflow succeeded'); + }); +} + +for (const target of ['Check', 'reply']) { + test(`an uncertain pending ${target} write cannot dispatch even if GitHub applies it after timeout`, {timeout: 2_000}, async t => { + shortenWebhookDeadline(t); + let lateApply, reached = false; + const f = await completionMock(t, {admit: false, async onCall(call, context) { + const pendingCheck = target === 'Check' && call.method === 'PATCH' && call.path.endsWith('/check-runs/77'); + const pendingReply = target === 'reply' && call.method === 'POST' && call.path.endsWith('/12/comments'); + if (!pendingCheck && !pendingReply) return; + reached = true; + lateApply = () => { + if (pendingCheck) Object.assign(context.check, call.body); + else context.comments.set(600, {id: 600, ...call.body}); + }; + await new Promise((resolve, reject) => { + call.signal.addEventListener('abort', () => reject(Error('ambiguous upstream write')), {once: true}); + }); + }}); + assert.equal((await f.submit()).status, 500);assert.equal(reached, true); + assert.equal(f.dispatches().length, 0);assert.equal(f.rows()[0].state, 'reserved'); + lateApply(); + assert.equal((await f.submit()).status, 200); + assert.equal(f.dispatches().length, 0);assert.equal(f.rows()[0].state, 'reserved'); + }); +} + +for (const conclusion of ['success', 'failure', 'cancelled', 'timed_out', 'neutral', 'skipped', 'stale', 'action_required', 'startup_failure']) { + test(`canonical ${conclusion} updates the same reply and releases only its request`, async t => { + const f = await completionMock(t);f.run.conclusion = conclusion; + const description = f.pr.body; + assert.equal((await f.complete()).status, 200); + const row = f.rows()[0], state = ['success', 'cancelled'].includes(conclusion) ? conclusion : 'failure'; + assert.equal(row.state, state);assert.equal(row.conclusion, conclusion); + assert.equal(row.completed_at, Date.parse(f.run.updated_at)); + assert.equal(f.pr.body, description);assert.equal(f.comments.size, 1); + assert.equal(row.status_comment_id, 600);assert.match(f.comments.get(600).body, /Workflow (succeeded|failed|cancelled)/); + assert.match(f.comments.get(600).body, /actions\/runs\/900/); + assert.match(f.comments.get(600).body, /pilot performs no dependency repair/); + assert.equal(f.check.external_id, `node-repair-request:${delivery}`); + assert.equal(f.check.status, 'completed');assert.equal(f.check.conclusion, 'neutral'); + assert.equal(f.idle.length, 1);assert.equal(f.idle[0].id, 78); + assert.equal(f.idle[0].actions[0].identifier, 'repair-selected'); + const permission = f.calls.filter(c => c.path.endsWith('/access_tokens')).at(-1).body; + assert.deepEqual(permission, {repository_ids: [7], permissions: {contents: 'read', pull_requests: 'write', checks: 'write', actions: 'read', issues: 'read'}}); + assert.equal((await f.complete()).status, 200); + assert.equal(f.idle.length, 1);assert.equal(f.dispatches().length, 1); + assert.equal((await f.submit()).status, 200);assert.equal(f.dispatches().length, 1); + }); +} + +test('native completion needs no conversation write grant', async t => { + const f = await completionMock(t, {native: true}); + assert.equal((await f.complete()).status, 200); + assert.equal(f.rows()[0].state, 'success'); + assert.deepEqual(f.calls.filter(c => c.path.endsWith('/access_tokens')).at(-1).body.permissions, + {contents: 'read', pull_requests: 'read', checks: 'write', actions: 'read'}); +}); + +for (const [name, mutate] of [ + ['run ID', f => {f.run.id = 901;}], + ['repository ID', f => {f.run.repository.id = 8;}], + ['repository name', f => {f.run.repository.full_name = 'other/repo';}], + ['workflow', f => {f.run.path = '.github/workflows/other.yml';}], + ['event', f => {f.run.event = 'push';}], + ['dispatched ref', f => {f.run.head_branch = 'other';}], + ['App identity', f => {f.app.id = 43;}], + ['actor type', f => {f.run.actor.type = 'User';}], + ['actor login', f => {f.run.actor.login = 'other[bot]';}], + ['run URL', f => {f.run.html_url = 'https://github.com/other/repo/actions/runs/900';}], + ['unfinished canonical run', f => {f.run.status = 'in_progress';}], +]) { + test(`mismatched canonical ${name} cannot publish or release`, async t => { + const f = await completionMock(t);mutate(f); + const before = f.calls.length; + assert.equal((await f.complete()).status, 200); + assert.equal(f.rows()[0].state, 'dispatched');assert.equal(f.rows()[0].conclusion, null); + assert.equal(f.calls.slice(before).filter(c => c.method === 'PATCH').length, 0); + }); +} + +test('unknown, early and cross-installation completions make no GitHub calls or changes', async t => { + const f = await completionMock(t); + for (const changes of [{workflow_run: {id: 901}}, {installation: {id: 4}}, {repository: {id: 8, full_name: 'fixture/repo'}}]) { + const count = f.calls.length; + assert.equal((await f.complete(changes)).status, 200);assert.equal(f.calls.length, count); + } + f.db.sql.prepare("UPDATE requests SET state = 'reserved', run_id = NULL, run_url = NULL, html_url = NULL").run(); + const count = f.calls.length; + assert.equal((await f.complete()).status, 200);assert.equal(f.calls.length, count); + assert.equal(f.rows()[0].state, 'reserved'); +}); + +test('a renamed default branch does not change the retained dispatch ref', async t => { + const f = await completionMock(t);f.repository.default_branch = 'trunk';f.pr.base.ref = 'trunk'; + assert.equal((await f.complete()).status, 200);assert.equal(f.rows()[0].state, 'success'); +}); + +for (const target of ['Check', 'reply']) { + test(`transient ${target} failure retains outcome and lock until redelivery`, async t => { + let armed = false; + const f = await completionMock(t, {onCall(call, {db}) { + if (armed && call.method === 'PATCH' && call.path.endsWith(target === 'Check' ? '/check-runs/77' : '/comments/600')) { + assert.equal(db.sql.prepare('SELECT state FROM requests').get().state, 'dispatched'); + assert.equal(db.sql.prepare('SELECT conclusion FROM requests').get().conclusion, 'success'); + return new Response(null, {status: 503}); + } + }}); + armed = true;assert.equal((await f.complete()).status, 500); + assert.equal(f.rows()[0].state, 'dispatched');assert.equal(f.rows()[0].conclusion, 'success'); + // Its saved outcome still holds the same active-index lock. + assert.throws(() => f.db.sql.prepare("INSERT INTO requests SELECT 'other', installation_id, repository_id, repository, pr, head, base, dispatched_ref, fingerprint, check_id, actor_login, created_at, NULL, state, NULL, NULL, NULL, NULL, NULL, NULL, NULL FROM requests").run(), /UNIQUE/); + armed = false;assert.equal((await f.complete()).status, 200);assert.equal(f.rows()[0].state, 'success'); + assert.equal(f.comments.size, 1);assert.equal(f.dispatches().length, 1); + }); +} + +for (const target of ['Check', 'reply']) { + for (const kind of ['missing', 'replaced']) { + test(`${kind} ${target} is not overwritten or recreated on verified completion`, async t => { + let armed = false; + const path = target === 'Check' ? '/repos/fixture/repo/check-runs/77' : '/repos/fixture/repo/issues/comments/600'; + const f = await completionMock(t, {onCall(call) { + if (!armed || call.path !== path || call.method !== 'GET') return; + return kind === 'missing' ? new Response(null, {status: 404}) : json(target === 'Check' ? ownCheck({app: {id: 43}}) : + {id: 600, performed_via_github_app: {id: 43}, body: `\n`, issue_url: 'https://api.github.com/repos/fixture/repo/issues/12'}); + }}); + armed = true;const before = f.calls.length; + assert.equal((await f.complete()).status, 200);assert.equal(f.rows()[0].state, 'success'); + assert.equal(f.calls.slice(before).filter(c => c.path === path && c.method === 'PATCH').length, 0); + assert.equal(f.comments.size, 1); + }); + } +} + +test('completion does not change a newer head Check', async t => { + const f = await completionMock(t);f.pr.head.sha = 'c'.repeat(40); + assert.equal((await f.complete()).status, 200);assert.equal(f.rows()[0].state, 'success');assert.equal(f.idle.length, 0); +}); + +test('conflicting canonical outcomes never rewrite a retained result', async t => { + const f = await completionMock(t);assert.equal((await f.complete()).status, 200); + const before = f.calls.length;f.run.conclusion = 'failure'; + assert.equal((await f.complete()).status, 500);assert.equal(f.rows()[0].state, 'success');assert.equal(f.rows()[0].conclusion, 'success'); + assert.equal(f.calls.slice(before).filter(c => c.method === 'PATCH').length, 0); +}); + +test('a fresh conversation command can start after completion without manual lock recovery', async t => { + let second = false; + const f = await completionMock(t, {onCall(call, context) { + if (!second) return; + if (call.path.endsWith('/dispatches')) return json({workflow_run_id: 901, + run_url: 'https://api.github.com/repos/fixture/repo/actions/runs/901', html_url: 'https://github.com/fixture/repo/actions/runs/901'}); + if (call.path.endsWith('/12/comments') && call.method === 'POST') { + const reply = {id: 601, ...call.body, performed_via_github_app: {id: 42}, issue_url: context.command.issue_url}; + context.comments.set(601, reply);return json(reply); + } + if (call.path.endsWith('/comments/601')) { + const reply = context.comments.get(601);if (call.method === 'PATCH') Object.assign(reply, call.body);return json(reply); + } + }}); + assert.equal((await f.complete()).status, 200);second = true; + f.command.id = 502; + assert.equal((await f.submit('a4e535a5-2a7d-491d-858a-3df51ce211df', {comment: {...f.command}})).status, 200); + assert.deepEqual(f.rows().map(r => r.state), ['success', 'dispatched']); + assert.deepEqual(f.rows().map(r => r.check_id), [77, 78]); + assert.equal(f.dispatches().length, 2);assert.match(f.comments.get(600).body, /succeeded/);assert.match(f.comments.get(601).body, /Request accepted/); +}); + +test('completion fits the shared eight-second budget with realistic API latency', {timeout: 12_000}, async t => { + let armed = false; + const f = await completionMock(t, {async onCall(call) { + if (!armed) return; + await new Promise((resolve, reject) => { + const abort = () => {clearTimeout(timer);reject(Error('fixture deadline'));}; + const timer = setTimeout(() => {call.signal.removeEventListener('abort', abort);resolve();}, 550); + if (call.signal.aborted) abort();else call.signal.addEventListener('abort', abort, {once: true}); + }); + }}); + armed = true;assert.equal((await f.complete()).status, 200);assert.equal(f.rows()[0].state, 'success'); +}); + +test('dispatch saves its run without further status writes; observers cannot repaint completion', async t => { + const f = await completionMock(t); + const before = f.calls.length; + assert.equal((await f.complete()).status, 200); + const terminal = structuredClone(f.check), reply = structuredClone(f.comments.get(600)); + const afterCompletion = f.calls.length; + await f.submit(); + await worker.fetch(signed({...payload, number: 12}, 'pull_request'), f.environment); + assert.deepEqual(f.check, terminal);assert.deepEqual(f.comments.get(600), reply); + assert.equal(f.calls.slice(afterCompletion).filter(c => c.method === 'PATCH').length, 0); + const writes = f.calls.slice(before).filter(c => c.method === 'PATCH'); + assert.ok(writes.every(c => c.body?.output?.title === 'Workflow succeeded' || c.body?.body?.includes('Workflow succeeded'))); + assert.equal(f.dispatches().length, 1); +}); + +test('an idle refresh delayed across admission and completion cannot repaint the terminal Check', {timeout: 3_000}, async t => { + const hold = Promise.withResolvers(), reached = Promise.withResolvers(); + let armed = true; + const f = await completionMock(t, {admit: false, async onCall(call, context) { + if (armed && call.method === 'GET' && call.path.includes('/commits/') && call.path.endsWith('/check-runs')) { + armed = false;const snapshot = structuredClone(context.check);reached.resolve();await hold.promise; + return json(listOf(snapshot)); + } + }}); + t.after(hold.resolve); + const refresh = worker.fetch(signed({...payload, number: 12}, 'pull_request'), f.environment); + await reached.promise; + assert.equal((await f.submit()).status, 200);assert.equal((await f.complete()).status, 200); + hold.resolve();assert.equal((await refresh).status, 200); + assert.equal(f.check.output.title, 'Workflow succeeded');assert.match(f.comments.get(600).body, /Workflow succeeded/); + assert.equal(f.calls.filter(c => c.method === 'PATCH' && c.body?.external_id?.startsWith('node-repair:')).length, 0); +}); + +test('ambiguous run ownership fails closed without changing either request', async t => { + const f = await completionMock(t); + const row = {...f.rows()[0], id: 'other-request', pr: 13, check_id: 79, comment_id: null, status_comment_id: null}; + const columns = Object.keys(row); + f.db.sql.prepare(`INSERT INTO requests (${columns.join(',')}) VALUES (${columns.map(() => '?').join(',')})`).run(...Object.values(row)); + const before = f.calls.length; + assert.equal((await f.complete()).status, 500);assert.equal(f.calls.length, before); + assert.deepEqual(f.rows().map(r => r.state), ['dispatched', 'dispatched']); +}); + +for (const change of [{conclusion: null}, {conclusion: 'invented'}, {updated_at: 'invalid'}]) { + test(`invalid canonical completion ${JSON.stringify(change)} retains the active lock`, async t => { + const f = await completionMock(t);Object.assign(f.run, change); + assert.equal((await f.complete()).status, 500);assert.equal(f.rows()[0].state, 'dispatched');assert.equal(f.rows()[0].conclusion, null); + }); +} + +test('daily retention deletes only terminal records older than ninety days', async t => { + const f = await completionMock(t), now = Date.parse('2026-10-24T12:00:00Z'), cutoff = now - 90 * 86_400_000; + f.db.sql.prepare('UPDATE requests SET completed_at = ?, conclusion = ?').run(cutoff - 1, 'success'); + for (const [index, state] of ['reserved', 'unknown', 'success', 'failure', 'cancelled'].entries()) { + const row = {...f.rows()[0], id: state, pr: 20 + index, state, completed_at: cutoff - 1, + comment_id: null, status_comment_id: null}; + const columns = Object.keys(row); + f.db.sql.prepare(`INSERT INTO requests (${columns.join(',')}) VALUES (${columns.map(() => '?').join(',')})`).run(...Object.values(row)); + } + const row = {...f.rows()[0], id: 'at-cutoff', pr: 30, state: 'success', completed_at: cutoff, comment_id: null, status_comment_id: null}; + const columns = Object.keys(row); + f.db.sql.prepare(`INSERT INTO requests (${columns.join(',')}) VALUES (${columns.map(() => '?').join(',')})`).run(...Object.values(row)); + await worker.scheduled({scheduledTime: now}, f.environment); + assert.deepEqual(f.rows().map(r => r.id).sort(), [delivery, 'reserved', 'unknown', 'at-cutoff'].sort()); + assert.equal(f.rows().find(r => r.id === delivery).conclusion, 'success'); + assert.equal(f.rows().find(r => r.id === delivery).state, 'dispatched'); +}); + test('a conversation command submits description selections and updates one App-owned reply', async t => { const f = await commentMock(t); assert.equal((await f.submit()).status, 200); assert.equal(f.dispatches().length, 1);assert.equal(f.rows().length, 1); assert.equal(f.rows()[0].comment_id, 501);assert.equal(f.rows()[0].status_comment_id, 600); assert.deepEqual(f.calls[0].body.permissions, {contents: 'read', pull_requests: 'write', checks: 'write', actions: 'write', issues: 'read'}); - assert.match(f.comments.get(600).body, /Workflow started/); + assert.match(f.comments.get(600).body, /Request accepted/); assert.match(f.comments.get(600).body, /launch-editor.*2\.14\.1 → 2\.14\.2/); - assert.match(f.comments.get(600).body, /actions\/runs\/900/); + assert.match(f.comments.get(600).body, /actions\/workflows\/dependency-repair\.yml/); const id = 'a4e535a5-2a7d-491d-858a-3df51ce211df'; assert.equal((await f.submit()).status, 200);assert.equal((await f.submit(id)).status, 200); assert.equal(f.calls.filter(c => c.path.endsWith('/12/comments') && c.method === 'POST').length, 1); @@ -790,9 +1042,9 @@ test('a conversation command finishes its status updates within the webhook dead assert.equal((await f.submit()).status, 200); assert.equal(f.rows().length, 1);assert.equal(f.dispatches().length, 1); assert.equal(f.rows()[0].state, 'dispatched'); - assert.match(f.check.output.summary, /actions\/runs\/900/); - assert.match(f.comments.get(600).body, /Workflow started/); - assert.match(f.comments.get(600).body, /actions\/runs\/900/); + assert.match(f.check.output.summary, /actions\/workflows\/dependency-repair\.yml/); + assert.match(f.comments.get(600).body, /Request accepted/); + assert.match(f.comments.get(600).body, /actions\/workflows\/dependency-repair\.yml/); }); for (const [name, mutate] of [ @@ -894,24 +1146,18 @@ test('an invalid reply owner retains the reservation without dispatch', async t assert.equal(f.rows()[0].status_comment_id, null);assert.equal(f.dispatches().length, 0); }); -test('a delayed reply refresh cannot erase the newly retained run link', {timeout: 2000}, async t => { - const dispatchReached = Promise.withResolvers(), finishDispatch = Promise.withResolvers(); - const replyReached = Promise.withResolvers(), finishReply = Promise.withResolvers(); - let dispatchPending = false, held = false; +test('replaying a command while dispatch is pending leaves its accepted reply untouched', {timeout: 2_000}, async t => { + const reached = Promise.withResolvers(), release = Promise.withResolvers(); const f = await commentMock(t, {async onCall(call) { - if (call.path.endsWith('/dispatches')) { - dispatchPending = true;dispatchReached.resolve();await finishDispatch.promise; - } else if (dispatchPending && !held && call.path.endsWith('/comments/600') && call.method === 'PATCH') { - held = true;replyReached.resolve();await finishReply.promise; - } + if (call.path.endsWith('/dispatches')) { reached.resolve();await release.promise; } }}); - const submit = f.submit();await dispatchReached.promise; - const replay = f.submit();await replyReached.promise; - finishDispatch.resolve();assert.equal((await submit).status, 200); - assert.match(f.comments.get(600).body, /actions\/runs\/900/); - finishReply.resolve();assert.equal((await replay).status, 200); - assert.match(f.comments.get(600).body, /Workflow started/); - assert.match(f.comments.get(600).body, /actions\/runs\/900/); + t.after(release.resolve); + const submit = f.submit();await reached.promise; + const reply = structuredClone(f.comments.get(600)); + assert.equal((await f.submit()).status, 200); + release.resolve();assert.equal((await submit).status, 200); + assert.deepEqual(f.comments.get(600), reply); + assert.equal(f.calls.filter(c => c.path.endsWith('/comments/600') && c.method === 'PATCH').length, 0); assert.equal(f.dispatches().length, 1); }); @@ -925,12 +1171,12 @@ test('a replaced status comment is not overwritten on delivery replay', async t const f = await commentMock(t);assert.equal((await f.submit()).status, 200); f.comments.get(600).performed_via_github_app.id = 99; const writes = f.calls.filter(c => c.path.endsWith('/comments/600') && c.method === 'PATCH').length; - assert.equal((await f.submit()).status, 500); + assert.equal((await f.submit()).status, 200); assert.equal(f.calls.filter(c => c.path.endsWith('/comments/600') && c.method === 'PATCH').length, writes); assert.equal(f.dispatches().length, 1); }); -test('a second comment updates the active conversation reply without another dispatch', async t => { +test('a second comment leaves the active conversation reply unchanged', async t => { const f = await commentMock(t);assert.equal((await f.submit()).status, 200); f.command.id = f.commentEvent.comment.id = 502; assert.equal((await f.submit('a4e535a5-2a7d-491d-858a-3df51ce211df')).status, 200); @@ -942,7 +1188,7 @@ test('an existing native request is explained in the conversation without a new assert.equal((await f.submit('a4e535a5-2a7d-491d-858a-3df51ce211df')).status, 200); assert.equal(f.dispatches().length, 1);assert.equal(f.rows().length, 1); assert.match(f.comments.get(600).body, /another repair request is active/); - assert.match(f.comments.get(600).body, /actions\/runs\/900/); + assert.match(f.comments.get(600).body, /actions\/workflows\/dependency-repair\.yml/); }); test('the source-comment index blocks a second UUID after the first request terminates', async t => { @@ -1077,40 +1323,24 @@ for (const dispatch of ['timeout', '204', '201', 'foreign']) { }); } -test('a failed in-progress Check update retains reserved state without dispatch', async t => { +test('a failed pending Check write retains reservation without dispatch or automatic repaint', async t => { let failed = false; const f = await actionMock(t, {onCall(call) { if (!failed && call.path.endsWith('/check-runs/77') && call.method === 'PATCH') { - failed = true; - return new Response('private', {status: 502}); + failed = true;return new Response('private', {status: 502}); } }}); - assert.equal((await f.click()).status, 500);assert.equal(f.rows()[0].state, 'reserved');assert.equal(f.dispatches().length, 0); - assert.equal((await f.click()).status, 200);assert.equal(f.dispatches().length, 0); - assert.equal(f.check.external_id, `node-repair-request:${delivery}`); - assert.equal(f.check.status, 'completed');assert.equal(f.check.conclusion, 'neutral');assert.deepEqual(f.check.actions, []); - assert.equal(f.check.output.title, 'Request reserved'); -}); - -test('a fresh click restores a same-Check reservation after the first Check write failed', async t => { - let failed = false; - const f = await actionMock(t, {onCall(call) { - if (!failed && call.path.endsWith('/check-runs/77') && call.method === 'PATCH') { - failed = true; - return new Response('private', {status: 502}); - } - }}); - assert.equal((await f.click()).status, 500); + assert.equal((await f.click()).status, 500);assert.equal(f.rows()[0].state, 'reserved'); + const writes = checkWrites(f.calls).length; + assert.equal((await f.click()).status, 200); assert.equal((await f.click('94e535a5-2a7d-491d-858a-3df51ce211de')).status, 200); - assert.equal(f.rows().length, 1);assert.equal(f.rows()[0].state, 'reserved');assert.equal(f.dispatches().length, 0); - assert.equal(f.check.external_id, `node-repair-request:${delivery}`);assert.deepEqual(f.check.actions, []); + assert.equal(f.dispatches().length, 0);assert.equal(checkWrites(f.calls).length, writes); }); test('replay cannot overwrite a foreign Check or regress a completed request', async t => { const f = await actionMock(t);assert.equal((await f.click()).status, 200); - f.check.app.id = 999; - const writes = checkWrites(f.calls).length; - assert.equal((await f.click()).status, 500);assert.equal(checkWrites(f.calls).length, writes); + f.check.app.id = 999;const writes = checkWrites(f.calls).length; + assert.equal((await f.click()).status, 200); f.db.sql.prepare("UPDATE requests SET state = 'success' WHERE id = ?").run(delivery); assert.equal((await f.click()).status, 200);assert.equal(checkWrites(f.calls).length, writes); assert.equal(f.dispatches().length, 1); @@ -1137,12 +1367,10 @@ test('oversized selected versions expose no action and reserve no request', asyn assert.deepEqual(writes.at(-1).body.actions, []); }); -test('failure after retaining the known run cannot erase its identity or dispatch again', async t => { - let writes = 0; - const f = await actionMock(t, {onCall(call) { - if (call.path.endsWith('/check-runs/77') && call.method === 'PATCH' && ++writes === 2) return new Response('private', {status: 502}); - }}); - assert.equal((await f.click()).status, 500);assert.equal(f.rows()[0].state, 'dispatched');assert.equal(f.rows()[0].run_id, 900); +test('retaining the known run performs no second Check write', async t => { + const f = await actionMock(t); + assert.equal((await f.click()).status, 200);assert.equal(f.rows()[0].run_id, 900); + assert.equal(checkWrites(f.calls).length, 1); assert.equal((await f.click()).status, 200);assert.equal(f.dispatches().length, 1); }); @@ -1166,7 +1394,7 @@ test('concurrent current clicks admit one request through the actual SQLite uniq test('D1-enabled PR edits show an action only for an eligible selected batch', async t => { const f = await actionMock(t); const refresh = () => worker.fetch(signed({...payload, number: 12, action: 'edited'}, 'pull_request'), f.environment); - assert.equal((await refresh()).status, 200);assert.equal(f.check.actions[0].identifier, 'repair-selected'); + assert.equal((await refresh()).status, 200);assert.equal(checkWrites(f.calls).length, 0); f.pr.body = fixtures.cases[1].body;assert.equal((await refresh()).status, 200); const writes = checkWrites(f.calls); assert.equal(writes.at(-1).method, 'POST');assert.deepEqual(writes.at(-1).body.actions, []); @@ -1189,73 +1417,31 @@ test('deadline after reservation prevents dispatch and preserves the ambiguous r assert.equal((await f.click()).status, 500);assert.equal(f.dispatches().length, 0);assert.equal(f.rows()[0].state, 'reserved'); }); -test('an idle refresh that races admission restores the reserved Check identity', async t => { - let raced = false; - const f = await actionMock(t, {onCall(call, {db}) { - if (!raced && call.path.endsWith('/check-runs/77') && call.method === 'PATCH' && call.body.external_id.startsWith('node-repair:')) { - raced = true; - db.sql.prepare(`INSERT INTO requests (id, installation_id, repository_id, repository, pr, head, base, - dispatched_ref, fingerprint, check_id, actor_login, created_at, state) - VALUES (?, 3, 7, 'fixture/repo', 12, ?, ?, 'main', ?, 77, 'maintainer', 1, 'reserved')`) - .run(delivery, 'a'.repeat(40), 'b'.repeat(40), '0'.repeat(64)); - } - }}); - assert.equal((await worker.fetch(signed({...payload, number: 12}, 'pull_request'), f.environment)).status, 200); - assert.equal(raced, true);assert.equal(f.check.external_id, `node-repair-request:${delivery}`); - assert.equal(f.check.status, 'completed');assert.equal(f.check.conclusion, 'neutral');assert.deepEqual(f.check.actions, []);assert.equal(f.dispatches().length, 0); -}); - -test('a different active Check blocks a fresh idle click and links the active request', async t => { +test('a different active Check blocks admission without writing either Check', async t => { const f = await actionMock(t); f.db.sql.prepare(`INSERT INTO requests (id, installation_id, repository_id, repository, pr, head, base, dispatched_ref, fingerprint, check_id, actor_login, created_at, state) VALUES (?, 3, 7, 'fixture/repo', 12, ?, ?, 'main', ?, 88, 'maintainer', 1, 'unknown')`) .run('94e535a5-2a7d-491d-858a-3df51ce211de', 'a'.repeat(40), 'b'.repeat(40), '0'.repeat(64)); assert.equal((await f.click()).status, 200);assert.equal(f.rows().length, 1);assert.equal(f.dispatches().length, 0); - assert.equal(f.check.output.title, 'Another request is active');assert.match(f.check.output.summary, /runs\/88/); - assert.deepEqual(f.check.actions, []); -}); - -test('an active request cannot authorize overwriting a replaced or foreign Check', async t => { - const f = await actionMock(t);assert.equal((await f.click()).status, 200); - f.check.app.id = 999; - const writes = checkWrites(f.calls).length; - assert.equal((await worker.fetch(signed({...payload, number: 12}, 'pull_request'), f.environment)).status, 500); - assert.equal(checkWrites(f.calls).length, writes);assert.equal(f.rows()[0].state, 'dispatched'); + assert.equal(checkWrites(f.calls).length, 0); }); for (const route of ['PR edit', 'delivery replay']) { - for (const pause of ['GET', 'PATCH']) { - test(`a ${route} paused on Check ${pause} cannot erase a newly retained run link`, async t => { - const dispatchReached = Promise.withResolvers(), finishDispatch = Promise.withResolvers(); - const restoreReached = Promise.withResolvers(), finishRestore = Promise.withResolvers(); - let dispatchPending = false, held = false; - const f = await actionMock(t, {async onCall(call) { - if (call.path.endsWith('/dispatches')) { - dispatchPending = true; dispatchReached.resolve(); await finishDispatch.promise; - } else if (dispatchPending && !held && call.path.endsWith('/check-runs/77') && call.method === pause) { - held = true; restoreReached.resolve(); await finishRestore.promise; - } - }}); - const click = f.click(); - await dispatchReached.promise; - const restore = route === 'PR edit' ? - worker.fetch(signed({...payload, number: 12, action: 'edited'}, 'pull_request'), f.environment) : f.click(); - await restoreReached.promise; - finishDispatch.resolve(); - assert.equal((await click).status, 200); - assert.equal(f.rows()[0].state, 'dispatched'); - assert.match(f.check.output.summary, /actions\/runs\/900/); - finishRestore.resolve(); - assert.equal((await restore).status, 200); - assert.equal(f.rows()[0].run_id, 900); - assert.equal(f.check.output.title, 'Workflow started'); - assert.match(f.check.output.summary, /actions\/runs\/900/); - assert.equal(f.check.external_id, `node-repair-request:${delivery}`); - assert.deepEqual(f.check.actions, []); - assert.equal(f.dispatches().length, 1); - }); - } + test(`a ${route} during dispatch does not write request status`, {timeout: 3_000}, async t => { + const reached = Promise.withResolvers(), release = Promise.withResolvers(); + const f = await actionMock(t, {async onCall(call) { + if (call.path.endsWith('/dispatches')) { reached.resolve();await release.promise; } + }}); + t.after(release.resolve); + const admission = f.click();await reached.promise; + const before = checkWrites(f.calls).length; + const response = route === 'PR edit' ? + await worker.fetch(signed({...payload, number: 12, action: 'edited'}, 'pull_request'), f.environment) : await f.click(); + assert.equal(response.status, 200);assert.equal(checkWrites(f.calls).length, before); + release.resolve();assert.equal((await admission).status, 200); + assert.equal(f.check.output.title, 'Request accepted');assert.equal(checkWrites(f.calls).length, before); + }); } for (const [total, reuse] of [[101, false], [201, true], [1000, true]]) { @@ -1270,10 +1456,12 @@ for (const [total, reuse] of [[101, false], [201, true], [1000, true]]) { const db = requestDatabase(); t.after(() => db.sql.close()); assert.equal((await worker.fetch(signed({...payload, number: 12}, 'pull_request'), {...env, DB: db})).status, 200); const writes = checkWrites(calls); - assert.equal(writes.length, 1); - assert.equal(writes[0].method, reuse ? 'PATCH' : 'POST'); - assert.equal(writes[0].path, '/repos/fixture/repo/check-runs' + (reuse ? '/78' : '')); - assert.equal(writes[0].body.actions[0].identifier, 'repair-selected'); + assert.equal(writes.length, reuse ? 0 : 1); + if (!reuse) { + assert.equal(writes[0].method, 'POST'); + assert.equal(writes[0].path, '/repos/fixture/repo/check-runs'); + assert.equal(writes[0].body.actions[0].identifier, 'repair-selected'); + } const pages = calls.filter(c => c.path.includes('/commits/')).map(c => new URL(c.url).searchParams.get('page')); assert.deepEqual(pages, Array.from({length: Math.ceil(total / 100)}, (_, i) => String(i + 1))); }); diff --git a/repair-app/wrangler.jsonc b/repair-app/wrangler.jsonc index a1e6b5f..21ae346 100644 --- a/repair-app/wrangler.jsonc +++ b/repair-app/wrangler.jsonc @@ -7,6 +7,7 @@ "preview": { "name": "replace-with-approved-preview-worker-name", "workers_dev": true, + "triggers": {"crons": ["17 3 * * *"]}, "d1_databases": [{ "binding": "DB", "database_name": "replace-with-approved-database-name",