diff --git a/.github/PR_AUTOMATION.md b/.github/PR_AUTOMATION.md index 623ac3930..6cb317069 100644 --- a/.github/PR_AUTOMATION.md +++ b/.github/PR_AUTOMATION.md @@ -7,6 +7,8 @@ They fail visibly before any reviewer starts when that prerequisite is missing, The review gate and final writer both read every page of current exact-head `CI` runs and authorize only the latest run ID and attempt. The triggering `workflow_run` event is authoritative for its own generation, so a run listing that still lags behind it cannot hide a green run. Both readers briefly re-list while the listing is behind instead of treating lag as staleness; a newer run ID or attempt still wins and is never retried. +The final lifecycle reconciliation receives the same observed CI event and uses bounded listing-lag retries for every other wake-up. +Workflow and per-pull-request mutation groups use non-canceling `queue: max` concurrency, so stale queued wake-ups revalidate live state instead of canceling active publication. Model analysis fails closed when the reviewable pull request diff exceeds the applicable evidence limit. The trusted `evidence-diff-attributes` policy represents reproducibly verified generated artifacts with binary-change markers. The automation posts guidance on the pull request instead of invoking a model with partial evidence. @@ -196,7 +198,7 @@ A model-suspected breaking change promotes `risk/low` to `risk/medium`. Path rules can add `scope/core`, `scope/web`, `scope/ffi`, and `scope/tooling`. The classifier controls `scope/cross-cutting`, `kind/technical-debt`, and documentation-only classification. -Automatic review runs for every non-draft pull request that passes the remaining gates. +Automatic review runs for every open, non-draft pull request that passes the remaining gates. Every non-bot author is eligible immediately, including first-time contributors; there is no prior-merge requirement. Automatic review requires successful CI for the exact classified head. After the first review, a later push starts the second review when CI succeeds for that new head. @@ -206,15 +208,18 @@ The classifier reports possible shared scope in `overlap`, using candidate title An unavailable or invalid exact-head classification or an attempted eligible review adds only `automation-failed`. `needs-review` means a human reviewer can act now and is the current next actor. `needs-author-action` means the pull request author is the current next actor. -The two next-actor labels are automation-owned and mutually exclusive. -Successful reviews with findings select `needs-author-action`, while successful reviews without findings select `needs-review`. -Successful normal classification can clear stale next-actor and failure labels before it dispatches review. -Successful review clears `automation-failed`. -Blocked and duplicate review routes preserve or clear next-actor state through their resolved state without creating a failure label. -With valid classification and green CI, legitimacy triage or `ai-reviewed/2` hands the pull request to `needs-review` without another model review. -Prompt label clearing and convergence for draft, queued, and other lifecycle transitions are deferred to the supersession and reconciliation follow-up. - -Bot-authored pull requests do not run automatic routes or label reconciliation. +Lifecycle reconciliation is the only writer of these mutually exclusive actor labels. +`automation-failed` is orthogonal and may coexist with `needs-author-action` only when independent exact-head CI failure requires author action. +Every successful app-owned exact-head review check stores a bounded, versioned machine-readable `findings` or `no-findings` receipt. +Reconciliation validates the check app, SHA, external ID, conclusion, receipt schema, and exact receipt keys before using that outcome. +The newest trusted successful exact-head check is canonical, including explicit forced reviews. +An existing successful check without a receipt fails closed and preserves actor labels until a compatible outcome is available. +With green exact-head CI, findings select `needs-author-action`, no findings select `needs-review`, and clean legitimacy or terminal handoff selects `needs-review`. +Missing, pending, or nonstandard terminal CI conclusions select neither actor. +Draft pull requests and active automation leases select neither actor. +Closed pull requests select neither actor, and reopening derives the actor again from current exact-head state. + +Bot-authored pull requests do not run automatic routes, but lifecycle reconciliation still clears invalid actor labels. Force mode can override policy gates for an open pull request at its current head after a trusted, valid classification for that exact head selects its reviewers. Force mode never bypasses classification validity, evidence retrieval, output validation, filesystem restrictions, protocol citation validation, or stale-head checks. @@ -224,12 +229,13 @@ Automatic classification and review claim their existing canonical check run bef The in-progress check contains only a versioned ownership marker with the kind, exact head SHA, workflow run ID, and run attempt. Admission reads all matching check history, accepts only the GitHub Actions app, and blocks while the marked owner is active. Canonical success is absorbing for an exact head, so later automatic or forced neutral and failure states do not change its check, labels, comments, or review count. -An automatic writer must still own its latest claim before it mutates the pull request, and completes that claim to success or neutral. +Classification admission, review admission, result publication, and lifecycle reconciliation serialize through one per-pull-request mutation boundary. +Automatic review validates the live PR, exact head, CI generation, policy, authoritative classification, and review count before claiming, then validates those facts with active lease ownership after the claim and before provider work or publication. This is deliberately at-least-once execution. If a run crashes after provider work, a later run may repeat the model call after the owner is terminal, canceled, or missing. -A failed final check or dispatch after earlier publication can require manual repair. -The automation does not recover artifacts, store receipts or a transaction ledger, or reconcile partial publication and lifecycle state. +A duplicate automatic review marker without a trusted successful receipt, or a failure after partial publication, stops fail-closed for manual repair. +The automation does not recover artifacts or store a transaction ledger. ## Size and fork limits @@ -245,9 +251,8 @@ Size uses the larger bucket from counted changed lines or touched files: | `size/XXL` | 1300 or more | 50 or more | `size/XXL` is informational and does not block classification or review. -The evidence diff limit is 1 MiB by default. -Adding `ai-review/allow-oversized` retries classification with the model runtime's maximum 4 MiB evidence limit. -Evidence above the applicable limit fails closed without sending a partial diff to a model. +The evidence diff limit is 4 MiB. +Evidence above that limit fails closed without sending a partial diff to a model. Fork-origin pull requests share a repository-wide quota of 50 pull requests per UTC day. `OWNER` and `MEMBER` pull requests are exempt and do not count toward the quota. @@ -257,9 +262,9 @@ Same-repository pull requests are also exempt. SHA-bound GitHub checks carry classification and review state between permission-isolated jobs. Workflow artifacts carry evidence and validated results between review-pipeline jobs. -The final writer serializes its mutations per pull request. +The final writer serializes its mutations per pull request and then reconciles actor labels from a fresh live snapshot. Workflow-level cancellation retains the established event and label grouping. -The final writer rechecks the current head, current review policy, and, for successful non-forced review or handoff, the latest exact-head CI generation. +The final writer rechecks the current head, current review policy, authoritative classification, lease, review count, and, for successful non-forced review or handoff, the latest exact-head CI generation. Model-execution jobs have read-only or empty permissions. The run summary links the pull request the run resolved. @@ -278,9 +283,15 @@ Detailed failure reasons appear only in the workflow summary. Every failed stage is reported, not only the first one. A mandatory specialist failure, invalid aggregate, invalid final review, exhausted limit, provider failure, or unavailable evidence applies only `automation-failed`. Stale heads stop publication without mutation. -Failed reviews do not increment the automated review count. +Failed reviews do not increment the automated review count, and successful reviews transition removal-first to exactly one count label so an interrupted transition undercounts rather than overcounts and may require manual repair. Cancelled runs do not publish fallback state. -This change intentionally provides no immediate lifecycle clearing when classification starts, CI starts, a pull request becomes draft, or another automation event is queued. +Events are wake-up and identity hints rather than actor-label transitions. +Classification and review result resolvers provide facts but do not write actor-label deltas. +An admitted automatic classifier that becomes draft publishes nothing, and a later ready event may retry it. +An admitted automatic review may publish its validated review, successful check, and review count while draft when its exact-head guards still pass, but it publishes no actor label. +An admitted automatic review may publish the same validated result on a closed unmerged pull request when its exact-head guards still pass, so reopening can reconcile from the receipt. +Merged pull requests and changed heads suppress publication. +If GitHub rejects review creation on a closed unmerged pull request, the lease completes neutral without publishing partial review state. ## Configuration and upgrades diff --git a/.github/pr-automation/automation-lease.js b/.github/pr-automation/automation-lease.js index 914f0a0cc..31da41f8e 100644 --- a/.github/pr-automation/automation-lease.js +++ b/.github/pr-automation/automation-lease.js @@ -5,6 +5,8 @@ const { readCheckRuns } = require("./check-runs"); const { matchesGeneration, readLatestExactHeadCiRun } = require("./ci-state"); const { reviewCount } = require("./resolve-state"); const { reviewPolicyEligible } = require("./routing"); +const { isClosedUnmerged, isOpenNonDraftAtHeadNow } = require("./current-head"); +const { parseCheckState } = require("./validate-classifier"); const APP = "github-actions"; const LEASE_PREFIX = "", review: review({ findings: [] }) }], + removeCommentMarkers: ["obsolete"], + check: { + name: "AI automated review", externalId: SHA, title: "Automated review complete", + summary: "Validated automated review.", outcome: "no-findings", + }, + }, + ciRetry: { retries: 0, delayMs: 0 }, + }), /marker cleanup failed/); + assert.deepEqual(calls, [ + ["review"], + ["labels", "ai-reviewed/1"], + ["check", "success"], + ["cleanup"], + ]); + assert.deepEqual([...labels].sort(), ["ai-reviewed/1", "risk/low"]); +}); + +function automaticReviewWriter({ + count = "ai-reviewed/1", existingReview = false, rejectReview = false, +} = {}) { + const lease = { + kind: "review", headSha: SHA, runId: 7, attempt: 1, checkRunId: 19, + marker: leaseMarker({ kind: "review", headSha: SHA, runId: 7, attempt: 1 }), + }; + const claim = { + id: 19, head_sha: SHA, external_id: SHA, status: "in_progress", + app: { slug: "github-actions" }, output: { summary: lease.marker }, + }; + const classification = { + id: 9, head_sha: SHA, external_id: `${CLASSIFIER_SCHEMA_VERSION}:${SHA}`, + conclusion: "success", app: { slug: "github-actions" }, + }; + const labels = new Set(["risk/low", count].filter(Boolean)); + let published = 0; + let closed = false; + const listReviews = () => {}; + const listChecks = () => {}; + const listRuns = () => {}; + const github = { + paginate: { iterator: async function* (method, parameters) { + if (method === listReviews) { + yield { data: existingReview ? [{ + user: { login: "github-actions[bot]" }, body: "", + }] : [] }; + } else if (method === listChecks) { + yield { data: parameters.check_name === "AI classification" ? [classification] : [claim] }; + } else if (method === listRuns) yield { data: [ciRun({ id: 1 })] }; + } }, + rest: { + actions: { + getWorkflowRun: async () => ({ data: { run_attempt: 1, status: "in_progress" } }), + listWorkflowRunsForRepo: listRuns, + }, + checks: { + listForRef: listChecks, + update: async ({ conclusion, output }) => Object.assign(claim, { + status: "completed", conclusion, output, + }), + }, + pulls: { + get: async () => ({ data: { + state: closed ? "closed" : "open", merged: false, merged_at: null, + draft: false, head: { sha: SHA }, + } }), + listReviews, + createReview: async () => { + if (rejectReview) { + closed = true; + const error = new Error("review endpoint rejects closed pull request"); + error.status = 422; + throw error; + } + published += 1; + }, + }, + issues: { + get: async () => ({ data: { labels: [...labels].map((name) => ({ name })) } }), + addLabels: async ({ labels: additions }) => additions.forEach((label) => labels.add(label)), + removeLabel: async ({ name }) => labels.delete(name), + }, + }, + }; + const nextCount = count === "ai-reviewed/1" ? "ai-reviewed/2" : "ai-reviewed/1"; + const state = { + ok: true, mode: "review", expectedSha: SHA, lease, expectedReviewCount: count, + nextReviewCount: nextCount, ciRunId: 1, ciRunAttempt: 1, + admittedGate: { + classificationId: 9, ciRunId: 1, ciRunAttempt: 1, + labels: [...labels], policyEligible: true, legitimacyStopped: false, + }, + labelSets: [{ owned: ["ai-reviewed/1", "ai-reviewed/2"], desired: [nextCount] }], + comments: [{ kind: "review", marker: "", review: review({ findings: [] }) }], + removeCommentMarkers: [], + check: { name: "AI automated review", externalId: SHA, outcome: "no-findings" }, + }; + const write = () => writeState({ + github, owner: "Devolutions", repo: "IronRDP", prNumber: 1, state, botLogin: "github-actions[bot]", + ciRetry: { retries: 0, delayMs: 0 }, + }); + return { claim, labels, published: () => published, write }; +} + +test("the second automatic review receives its receipt at the terminal count and cannot advance twice", async () => { + const writer = automaticReviewWriter(); + await writer.write(); + assert.deepEqual({ + published: writer.published(), labels: [...writer.labels].sort(), conclusion: writer.claim.conclusion, + }, { + published: 1, labels: ["ai-reviewed/2", "risk/low"], conclusion: "success", + }); + await writer.write(); + assert.deepEqual({ published: writer.published(), labels: [...writer.labels].sort() }, { + published: 1, labels: ["ai-reviewed/2", "risk/low"], + }); +}); + +test("an existing automatic marker without a receipt leaves a partial first review for manual repair", async () => { + const writer = automaticReviewWriter({ existingReview: true }); + const result = await writer.write(); + assert.deepEqual({ + superseded: result.superseded, published: writer.published(), + labels: [...writer.labels].sort(), conclusion: writer.claim.conclusion, + title: writer.claim.output.title, + }, { + superseded: true, published: 0, labels: ["ai-reviewed/1", "risk/low"], + conclusion: "neutral", title: "Review publication requires manual repair", + }); +}); + +test("a closed-unmerged review rejection neutralizes its lease before count or receipt publication", async () => { + const writer = automaticReviewWriter({ count: null, rejectReview: true }); + const result = await writer.write(); + assert.equal(result.superseded, true); + assert.deepEqual({ + labels: [...writer.labels], conclusion: writer.claim.conclusion, title: writer.claim.output.title, + }, { + labels: ["risk/low"], conclusion: "neutral", title: "Review publication unavailable", + }); +}); + test("writer applies blocked reconciliation without review-policy assertions", async () => { const baseGate = { ok: true, head_sha: SHA, classificationCheck: true, ciGreen: true, @@ -3530,103 +4097,43 @@ test("writer applies blocked reconciliation without review-policy assertions", a if (state.reason === "an automated review already exists for this head") { assert.deepEqual(removed, []); } else { - assert.deepEqual(removed.sort(), ["needs-author-action", "needs-review"]); + assert.deepEqual(removed, []); } } }); -test("writer retries a truncated current-head read before dispatching once", async () => { - let reads = 0; - let checkWrites = 0; - let dispatches = 0; - const machineState = { - protocolRelated: false, risk: "low", specialistReviewers: [], - automaticReviewEligible: true, - }; - const state = { - ok: true, mode: "classification", expectedSha: SHA, labelSets: [], addLabels: [], - comments: [], removeCommentMarkers: [], dispatchReview: true, - check: { - name: "AI classification", externalId: `${CLASSIFIER_SCHEMA_VERSION}:${SHA}`, - title: "Classification complete", summary: "Validated classification.", machineState, - }, - }; - const github = { - paginate: { iterator: async function* () { yield { data: [] }; } }, - rest: { - checks: { listForRef: () => {}, create: async () => { checkWrites += 1; } }, - pulls: { get: async () => { - reads += 1; - if (reads === 3) { - const error = new Error("Unexpected end of JSON input"); - error.status = 500; - throw error; - } - return { data: { state: "open", head: { sha: SHA } } }; - } }, - issues: { get: async () => ({ data: { labels: [] } }) }, - repos: { createDispatchEvent: async () => { dispatches += 1; } }, - }, +test("current-head reads retry one truncated GitHub response", async () => { + const verify = async (responses) => { + let reads = 0; + const github = { rest: { pulls: { get: async () => { + const response = responses[reads++]; + if (response instanceof Error) throw response; + return { data: { state: "open", head: { sha: response } } }; + } } } }; + const result = assertCurrentHead({ + github, owner: "Devolutions", repo: "IronRDP", pullNumber: 1, expectedHeadSha: SHA, + }); + return { result, reads: () => reads }; }; - await writeState({ - github, owner: "Devolutions", repo: "IronRDP", prNumber: 1, botLogin: "github-actions[bot]", - state, - }); - assert.equal(reads, 4); - assert.equal(checkWrites, 1); - assert.equal(dispatches, 1); + const error = (message) => Object.assign(new Error(message), { status: 500 }); - const failedDispatch = async (errorAtRead) => { - let failedReads = 0; - let failedCheckWrites = 0; - let failedDispatches = 0; - const github = { - paginate: { iterator: async function* () { yield { data: [] }; } }, - rest: { - checks: { listForRef: () => {}, create: async () => { failedCheckWrites += 1; } }, - pulls: { get: async () => { - failedReads += 1; - if (failedReads > 2) { - const result = errorAtRead(failedReads); - if (result instanceof Error) throw result; - return { data: { state: "open", head: { sha: result } } }; - } - return { data: { state: "open", head: { sha: SHA } } }; - } }, - issues: { get: async () => ({ data: { labels: [] } }) }, - repos: { createDispatchEvent: async () => { failedDispatches += 1; } }, - }, - }; - await assert.rejects(writeState({ - github, owner: "Devolutions", repo: "IronRDP", prNumber: 1, botLogin: "github-actions[bot]", - state, - })); - return { failedReads, failedCheckWrites, failedDispatches }; - }; + const recovered = await verify([error("Unexpected end of JSON input"), SHA]); + await assert.doesNotReject(recovered.result); + assert.equal(recovered.reads(), 2); - const terminal = await failedDispatch(() => { - const error = new Error("internal server error"); - error.status = 500; - return error; - }); - assert.deepEqual(terminal, { failedReads: 3, failedCheckWrites: 1, failedDispatches: 0 }); + const terminal = await verify([error("internal server error")]); + await assert.rejects(terminal.result, /internal server error/); + assert.equal(terminal.reads(), 1); - const staleRetry = await failedDispatch((read) => { - if (read === 3) { - const error = new Error("Unexpected end of JSON input"); - error.status = 500; - return error; - } - return OTHER_SHA; - }); - assert.deepEqual(staleRetry, { failedReads: 4, failedCheckWrites: 1, failedDispatches: 0 }); + const stale = await verify([error("Unexpected end of JSON input"), OTHER_SHA]); + await assert.rejects(stale.result, StaleHeadError); + assert.equal(stale.reads(), 2); - const exhausted = await failedDispatch(() => { - const error = new Error("Unexpected end of JSON input"); - error.status = 500; - return error; - }); - assert.deepEqual(exhausted, { failedReads: 4, failedCheckWrites: 1, failedDispatches: 0 }); + const exhausted = await verify([ + error("Unexpected end of JSON input"), error("Unexpected end of JSON input"), + ]); + await assert.rejects(exhausted.result, /Unexpected end of JSON input/); + assert.equal(exhausted.reads(), 2); }); test("writer does not dispatch a completed classification after the head changes", async () => { @@ -3696,10 +4203,12 @@ test("writer deduplicates one forced review invocation but publishes a later one }, }; await writeState({ - github, owner: "Devolutions", repo: "IronRDP", prNumber: 1, botLogin: "github-actions[bot]", + github: withCurrentValidClassification(github), owner: "Devolutions", repo: "IronRDP", + prNumber: 1, botLogin: "github-actions[bot]", state: { ok: true, mode: "review", expectedSha: SHA, labelSets: [], addLabels: [], expectedReviewCount: null, forced: true, protocolRelated: true, + admittedGate: { classificationId: FORCED_CLASSIFICATION_ID }, comments: [{ kind: "review", marker, review: review() }], }, }); @@ -3710,6 +4219,49 @@ test("writer deduplicates one forced review invocation but publishes a later one assert.equal(await publish(``), 1); }); +test("a newer same-head classification suppresses a forced review from the old route", async () => { + let published = 0; + const listChecks = () => {}; + const machineState = { + protocolRelated: false, risk: "low", specialistReviewers: [], automaticReviewEligible: false, + }; + const github = { + paginate: { iterator: async function* (method, parameters) { + if (method === listChecks) yield { data: [{ + id: 10, head_sha: SHA, external_id: `${CLASSIFIER_SCHEMA_VERSION}:${SHA}`, + conclusion: "success", app: { slug: "github-actions" }, + output: { summary: encodeCheckState(machineState) }, + }, { + id: 9, head_sha: SHA, external_id: `${CLASSIFIER_SCHEMA_VERSION}:${SHA}`, + conclusion: "success", app: { slug: "github-actions" }, + output: { summary: encodeCheckState(machineState) }, + }] }; + } }, + rest: { + checks: { listForRef: listChecks }, + pulls: { + get: async () => ({ data: { state: "open", draft: false, head: { sha: SHA } } }), + listReviews: () => {}, + createReview: async () => { published += 1; }, + }, + issues: { get: async () => ({ data: { labels: [] } }) }, + }, + }; + const result = await writeState({ + github, owner: "Devolutions", repo: "IronRDP", prNumber: 1, botLogin: "github-actions[bot]", + state: { + ok: true, mode: "review", expectedSha: SHA, forced: true, expectedReviewCount: null, + admittedGate: { classificationId: 9 }, + labelSets: [], comments: [{ + kind: "review", marker: ``, review: review(), + }], + removeCommentMarkers: [], + }, + }); + assert.equal(result.superseded, true); + assert.equal(published, 0); +}); + test("failed review publication does not consume review count or change triage", async () => { let labelWrites = 0; const github = { @@ -3728,10 +4280,12 @@ test("failed review publication does not consume review count or change triage", }, }; await assert.rejects(writeState({ - github, owner: "Devolutions", repo: "IronRDP", prNumber: 1, botLogin: "github-actions[bot]", + github: withCurrentValidClassification(github), owner: "Devolutions", repo: "IronRDP", + prNumber: 1, botLogin: "github-actions[bot]", state: { ok: true, mode: "review", expectedSha: SHA, expectedReviewCount: null, forced: true, protocolRelated: false, + admittedGate: { classificationId: FORCED_CLASSIFICATION_ID }, labelSets: [{ owned: ["ai-reviewed/1", "ai-reviewed/2"], desired: ["ai-reviewed/1"] }], addLabels: [], removeLabels: ["needs-review"], comments: [{ @@ -3763,10 +4317,12 @@ test("a final review check failure may require manual publication repair", async }, }; await assert.rejects(writeState({ - github, owner: "Devolutions", repo: "IronRDP", prNumber: 1, botLogin: "github-actions[bot]", + github: withCurrentValidClassification(github), owner: "Devolutions", repo: "IronRDP", + prNumber: 1, botLogin: "github-actions[bot]", state: { ok: true, mode: "review", expectedSha: SHA, expectedReviewCount: null, forced: true, protocolRelated: false, + admittedGate: { classificationId: FORCED_CLASSIFICATION_ID }, labelSets: [{ owned: ["ai-reviewed/1", "ai-reviewed/2"], desired: ["ai-reviewed/1"] }], addLabels: [], comments: [], check: { name: "AI automated review", externalId: SHA }, @@ -3789,10 +4345,12 @@ test("writer publishes each finding either inline or in the review body", async }, }; await writeState({ - github, owner: "Devolutions", repo: "IronRDP", prNumber: 1, botLogin: "github-actions[bot]", + github: withCurrentValidClassification(github), owner: "Devolutions", repo: "IronRDP", + prNumber: 1, botLogin: "github-actions[bot]", state: { ok: true, mode: "review", expectedSha: SHA, labelSets: [], addLabels: [], expectedReviewCount: null, forced: true, protocolRelated: true, + admittedGate: { classificationId: FORCED_CLASSIFICATION_ID }, comments: [{ kind: "review", marker: ``, @@ -3870,6 +4428,7 @@ test("writer rejects tampered publication payloads before calling GitHub", async const state = { ok: true, mode: "review", expectedSha: SHA, labelSets: [], addLabels: [], expectedReviewCount: null, forced: true, protocolRelated: false, + admittedGate: { classificationId: FORCED_CLASSIFICATION_ID }, comments: [{ kind: "review", marker: ``, @@ -3877,11 +4436,11 @@ test("writer rejects tampered publication payloads before calling GitHub", async }], }; await assert.rejects(writeState({ - github, owner: "Devolutions", repo: "IronRDP", prNumber: 1, + github: withCurrentValidClassification(github), owner: "Devolutions", repo: "IronRDP", prNumber: 1, botLogin: "github-actions[bot]", state, }), /review publication exceeds GitHub body limit/); await assert.rejects(writeState({ - github, owner: "Devolutions", repo: "IronRDP", prNumber: 1, + github: withCurrentValidClassification(github), owner: "Devolutions", repo: "IronRDP", prNumber: 1, botLogin: "github-actions[bot]", state: { ...state, @@ -3900,7 +4459,7 @@ test("writer rejects tampered publication payloads before calling GitHub", async }, }; await assert.rejects(writeState({ - github: existingGithub, owner: "Devolutions", repo: "IronRDP", prNumber: 1, + github: withCurrentValidClassification(existingGithub), owner: "Devolutions", repo: "IronRDP", prNumber: 1, botLogin: "github-actions[bot]", state, }), /review publication exceeds GitHub body limit/); assert.equal(published, false); @@ -3920,10 +4479,12 @@ test("writer publishes a green main comment when no findings remain", async () = }, }; await writeState({ - github, owner: "Devolutions", repo: "IronRDP", prNumber: 1, botLogin: "github-actions[bot]", + github: withCurrentValidClassification(github), owner: "Devolutions", repo: "IronRDP", + prNumber: 1, botLogin: "github-actions[bot]", state: { ok: true, mode: "review", expectedSha: SHA, labelSets: [], addLabels: [], expectedReviewCount: null, forced: true, protocolRelated: false, + admittedGate: { classificationId: FORCED_CLASSIFICATION_ID }, comments: [{ kind: "review", marker: ``, @@ -3950,10 +4511,12 @@ test("writer adds deterministic reduced-coverage notices without filtering findi }, }; await writeState({ - github, owner: "Devolutions", repo: "IronRDP", prNumber: 1, botLogin: "github-actions[bot]", + github: withCurrentValidClassification(github), owner: "Devolutions", repo: "IronRDP", + prNumber: 1, botLogin: "github-actions[bot]", state: { ok: true, mode: "review", expectedSha: SHA, labelSets: [], addLabels: [], expectedReviewCount: null, forced: true, protocolRelated: false, + admittedGate: { classificationId: FORCED_CLASSIFICATION_ID }, comments: [{ kind: "review", marker: ``, reducedCoverage: ["protocol"], @@ -5165,7 +5728,7 @@ test("the reusable pipeline stays caller-driven and reports every stage back", ( assert.doesNotMatch(triggers, /\n {2}(pull_request|push|schedule|workflow_dispatch|issue_comment):/); for (const input of ["pr-number", "head-sha", "base-sha", "specialist-reviewers", - "evidence-max-bytes", "required-reviewers", "gate", "lease"]) { + "required-reviewers", "gate", "lease"]) { assert.match(workflow, new RegExp(`\\n {6}${input}:\\n`), `${input} input is missing`); } for (const removed of ["prior-results", "recovery-attempt", "provenance"]) { @@ -5180,14 +5743,30 @@ test("the reusable pipeline stays caller-driven and reports every stage back", ( test("automatic leases claim once, read complete history, and block an active owner", async () => { const runs = []; const calls = { create: [], list: [] }; + const listChecks = () => {}; + const listRuns = () => {}; + const gate = { + ok: true, force: false, head_sha: SHA, classificationValid: true, + classificationCheck: true, classificationId: 9, legitimacyStopped: false, + ciGreen: true, ciRunId: 1, ciRunAttempt: 1, ciSource: "listing", + secondReviewEligible: true, reviewAtHead: false, policyEligible: true, + labels: ["risk/low"], contributor: { status: "eligible", association: "CONTRIBUTOR" }, + protocolRelated: false, risk: "low", specialistReviewers: ["code-compressor"], + }; const github = { - paginate: { iterator: async function* (_method, parameters) { + paginate: { iterator: async function* (method, parameters) { calls.list.push(parameters); - yield { data: runs }; + if (method === listRuns) yield { data: [ciRun({ id: 1 })] }; + else if (parameters.check_name === "AI classification") { + yield { data: [{ + id: 9, head_sha: SHA, external_id: `${CLASSIFIER_SCHEMA_VERSION}:${SHA}`, + conclusion: "success", app: { slug: "github-actions" }, + }] }; + } else yield { data: runs }; } }, rest: { checks: { - listForRef: () => {}, + listForRef: listChecks, create: async (payload) => { calls.create.push(payload); const claim = { @@ -5199,16 +5778,19 @@ test("automatic leases claim once, read complete history, and block an active ow }, actions: { getWorkflowRun: async () => ({ data: { run_attempt: 1, status: "in_progress" } }), + listWorkflowRunsForRepo: listRuns, }, + pulls: { get: async () => ({ data: { state: "open", draft: false, head: { sha: SHA } } }) }, + issues: { get: async () => ({ data: { labels: [{ name: "risk/low" }] } }) }, }, }; const first = await claimAutomaticLease({ github, owner: "Devolutions", repo: "IronRDP", kind: "review", headSha: SHA, - runId: 7, attempt: 1, + prNumber: 1, gate, runId: 7, attempt: 1, }); const duplicate = await claimAutomaticLease({ github, owner: "Devolutions", repo: "IronRDP", kind: "review", headSha: SHA, - runId: 8, attempt: 1, + prNumber: 1, gate, runId: 8, attempt: 1, }); assert.equal(first.owner, true); assert.equal(duplicate.owner, false); @@ -5221,6 +5803,53 @@ test("automatic leases claim once, read complete history, and block an active ow }); }); +test("automatic admission rereads live eligibility before creating a lease", async () => { + let claims = 0; + const github = { + paginate: { iterator: async function* () { yield { data: [] }; } }, + rest: { + pulls: { get: async () => ({ data: { state: "open", draft: true, head: { sha: SHA } } }) }, + checks: { listForRef: () => {}, create: async () => { claims += 1; } }, + }, + }; + const decision = await claimAutomaticLease({ + github, owner: "Devolutions", repo: "IronRDP", kind: "classification", headSha: SHA, + prNumber: 1, runId: 7, attempt: 1, + }); + assert.equal(decision.owner, false); + assert.equal(decision.reason, "pull request is no longer eligible"); + assert.equal(claims, 0); +}); + +test("automatic admission neutralizes a claim when post-validation observes a draft", async () => { + let pullReads = 0; + const updates = []; + const github = { + paginate: { iterator: async function* () { yield { data: [] }; } }, + rest: { + pulls: { get: async () => ({ + data: { + state: "open", draft: ++pullReads === 2, head: { sha: SHA }, + }, + }) }, + checks: { + listForRef: () => {}, + create: async () => ({ data: { id: 19 } }), + update: async (payload) => updates.push(payload), + }, + }, + }; + const decision = await claimAutomaticLease({ + github, owner: "Devolutions", repo: "IronRDP", kind: "classification", headSha: SHA, + prNumber: 1, runId: 7, attempt: 1, + }); + assert.equal(decision.owner, false); + assert.equal(decision.reason, "pull request changed during lease claim"); + assert.deepEqual(updates.map(({ check_run_id, conclusion, output }) => ({ + check_run_id, conclusion, title: output.title, + })), [{ check_run_id: 19, conclusion: "neutral", title: "Automation superseded" }]); +}); + test("automatic leases rerun after terminal or missing owners and fail closed on malformed claims", async () => { const marker = leaseMarker({ kind: "classification", headSha: SHA, runId: 7, attempt: 1 }); const admit = async ({ status = "completed", missing = false, summary = marker, app = "github-actions" } = {}) => { @@ -5373,8 +6002,9 @@ test("admission jobs own provider routing and recheck prevents a general call af assert.match(classificationAdmission, /actions: read/); assert.match(classificationAdmission, /checks: write/); assert.match(classificationAdmission, - /group: pr-automation-admission-classification-\$\{\{ needs\.resolve-pr\.outputs\.pr-number \}\}/); + /group: pr-automation-mutation-\$\{\{ needs\.resolve-pr\.outputs\.pr-number \}\}/); assert.match(classificationAdmission, /cancel-in-progress: false/); + assert.match(classificationAdmission, /queue: max/); assert.match(classificationAdmission, /ALLOW_SUCCESS:/); assert.match(classificationAdmission, /allowSuccess:/); assert.match(classifier, /classification-admission/); @@ -5383,8 +6013,13 @@ test("admission jobs own provider routing and recheck prevents a general call af assert.match(reviewAdmission, /actions: read/); assert.match(reviewAdmission, /checks: write/); assert.match(reviewAdmission, - /group: pr-automation-admission-review-\$\{\{ needs\.resolve-pr\.outputs\.pr-number \}\}/); + /group: pr-automation-mutation-\$\{\{ needs\.resolve-pr\.outputs\.pr-number \}\}/); assert.match(reviewAdmission, /cancel-in-progress: false/); + assert.match(reviewAdmission, /queue: max/); + const writer = workflowJob(workflow, "write-state"); + assert.match(writer, /group: pr-automation-mutation-\$\{\{ needs\.resolve-pr\.outputs\.pr-number \}\}/); + assert.match(writer, /cancel-in-progress: false/); + assert.match(writer, /queue: max/); assert.match(pipeline, /review-admission\.outputs\.owner == 'true'/); assert.match(pipeline, /lease: \$\{\{ needs\.review-admission\.outputs\.lease \}\}/); assert.match(workflowJob(reusable, "general"), /needs: \[evidence, aggregate, recheck\]/); @@ -5534,7 +6169,7 @@ test("forced failure cannot terminate an active automatic lease", async () => { assert.deepEqual(mutations, []); }); -test("a leased review failure mutates labels before completing its neutral check", async () => { +test("a leased review failure mutates only the failure label before its neutral check", async () => { const lease = { kind: "review", headSha: SHA, runId: 7, attempt: 1, checkRunId: 19, marker: leaseMarker({ kind: "review", headSha: SHA, runId: 7, attempt: 1 }), @@ -5566,10 +6201,7 @@ test("a leased review failure mutates labels before completing its neutral check github, owner: "Devolutions", repo: "IronRDP", prNumber: 1, state: { ok: true, mode: "review", expectedSha: SHA, lease, failed: true, - labelSets: [ - { owned: ["needs-review", "needs-author-action"], desired: [] }, - { owned: [FAILURE_LABEL], desired: [FAILURE_LABEL] }, - ], + labelSets: [{ owned: [FAILURE_LABEL], desired: [FAILURE_LABEL] }], comments: [], check: { name: "AI automated review", externalId: SHA, @@ -5578,13 +6210,12 @@ test("a leased review failure mutates labels before completing its neutral check }, }); assert.deepEqual(calls, [ - ["remove", "needs-review"], ["add", FAILURE_LABEL], ["check", "neutral"], ]); }); -test("a leased blocked review clears actors before completing its claim", async () => { +test("a leased blocked review completes its claim without actor-label mutation", async () => { const lease = { kind: "review", headSha: SHA, runId: 7, attempt: 1, checkRunId: 19, marker: leaseMarker({ kind: "review", headSha: SHA, runId: 7, attempt: 1 }), @@ -5615,14 +6246,11 @@ test("a leased blocked review clears actors before completing its claim", async github, owner: "Devolutions", repo: "IronRDP", prNumber: 1, state: { ok: true, mode: "review", expectedSha: SHA, lease, blocked: true, - labelSets: [{ owned: ["needs-review", "needs-author-action"], desired: [] }], + labelSets: [], comments: [], }, }); - assert.deepEqual(calls, [ - ["remove", "needs-review"], - ["check", "Automation blocked"], - ]); + assert.deepEqual(calls, [["check", "Automation blocked"]]); }); test("an automatic writer completes only its current claim and upgrades it to success", async () => { @@ -5753,9 +6381,11 @@ test("a forced same-head success still publishes beside an existing canonical su }, }; await writeState({ - github, owner: "Devolutions", repo: "IronRDP", prNumber: 1981, botLogin: "github-actions[bot]", + github: withCurrentValidClassification(github), owner: "Devolutions", repo: "IronRDP", + prNumber: 1981, botLogin: "github-actions[bot]", state: { ok: true, mode: "review", expectedSha: SHA, forced: true, expectedReviewCount: null, + admittedGate: { classificationId: FORCED_CLASSIFICATION_ID }, labelSets: [], comments: [{ kind: "review", marker: ``, review: review(), @@ -5804,11 +6434,12 @@ test("forced success neutralizes an overlapping automatic lease before publishin }, }; await writeState({ - github, owner: "Devolutions", repo: "IronRDP", prNumber: 1, + github: withCurrentValidClassification(github), owner: "Devolutions", repo: "IronRDP", prNumber: 1, botLogin: "github-actions[bot]", state: { ok: true, mode: "review", expectedSha: SHA, forced: true, - expectedReviewCount: null, labelSets: [], comments: [{ + expectedReviewCount: null, admittedGate: { classificationId: FORCED_CLASSIFICATION_ID }, + labelSets: [], comments: [{ kind: "review", marker: ``, review: review(), }], diff --git a/.github/pr-automation/ci-state.js b/.github/pr-automation/ci-state.js index fb5576b63..a8b34c44d 100644 --- a/.github/pr-automation/ci-state.js +++ b/.github/pr-automation/ci-state.js @@ -16,16 +16,27 @@ function exactHeadCiGeneration(run, expectedSha) { return run?.name === "CI" && run?.head_sha === expectedSha ? generation(run) : null; } -// The triggering `workflow_run` event is authoritative for its own generation because the run -// listing can lag behind it. A newer generation in the listing still wins. +function statusRank(status) { + switch (status) { + case "completed": return 3; + case "in_progress": return 2; + // `requested` is a workflow_run event action; `queued` is the corresponding run status. + case "queued": + case "requested": return 1; + default: return 0; + } +} + +// The triggering `workflow_run` event can be ahead of the listing. For the same generation, +// retain whichever source reports the more advanced GitHub workflow status. A newer generation +// in the listing still wins regardless of status. function latestExactHeadCiRun(runs, expectedSha, observedRun = null) { const observed = exactHeadCiGeneration(observedRun, expectedSha); - const candidates = (runs || []).filter((run) => { - const current = exactHeadCiGeneration(run, expectedSha); - return current && !(observed && compareGenerations(current, observed) === 0); - }); + const candidates = (runs || []).filter((run) => exactHeadCiGeneration(run, expectedSha)); if (observed) candidates.push(observedRun); - return candidates.sort((left, right) => compareGenerations(generation(right), generation(left)))[0] ?? null; + return candidates.sort((left, right) => + compareGenerations(generation(right), generation(left)) || + statusRank(right.status) - statusRank(left.status))[0] ?? null; } // Re-lists only while the listing is behind: no candidate, a candidate older than the expected diff --git a/.github/pr-automation/classification-gate.js b/.github/pr-automation/classification-gate.js index 574c214fb..e1d78a924 100644 --- a/.github/pr-automation/classification-gate.js +++ b/.github/pr-automation/classification-gate.js @@ -4,7 +4,7 @@ const { SCHEMA_VERSION, parseCheckState } = require("./validate-classifier"); const { readCheckRuns } = require("./check-runs"); async function resolveClassificationGate({ - github, owner, repo, expectedSha, force = false, retryWithLargerEvidence = false, + github, owner, repo, expectedSha, force = false, }) { if (force) { return { available: true, required: true, reason: "", force: true }; @@ -22,7 +22,7 @@ async function resolveClassificationGate({ }); return { available: true, - required: retryWithLargerEvidence || !completed, + required: !completed, reason: "", externalId, completed, diff --git a/.github/pr-automation/current-head.js b/.github/pr-automation/current-head.js index 60158236c..eb830e5fc 100644 --- a/.github/pr-automation/current-head.js +++ b/.github/pr-automation/current-head.js @@ -21,11 +21,30 @@ async function readPullRequest({ github, owner, repo, pullNumber }) { } } -async function assertCurrentHead({ github, owner, repo, pullNumber, expectedHeadSha }) { +function isClosedUnmerged(pull) { + return pull.state === "closed" && !pull.merged && !pull.merged_at; +} + +async function assertCurrentHead({ + github, owner, repo, pullNumber, expectedHeadSha, allowClosedUnmerged = false, +}) { const data = await readPullRequest({ github, owner, repo, pullNumber }); - if (data.state !== "open" || data.head?.sha !== expectedHeadSha) { + if ((data.state !== "open" && !(allowClosedUnmerged && isClosedUnmerged(data))) || + data.head?.sha !== expectedHeadSha) { throw new StaleHeadError(); } + return data; +} + +function isOpenNonDraftAtHead(pull, expectedHeadSha) { + return pull?.state === "open" && pull.draft !== true && pull.head?.sha === expectedHeadSha; +} + +async function isOpenNonDraftAtHeadNow({ github, owner, repo, pullNumber, expectedHeadSha }) { + const data = await readPullRequest({ github, owner, repo, pullNumber }); + return isOpenNonDraftAtHead(data, expectedHeadSha); } -module.exports = { StaleHeadError, assertCurrentHead }; +module.exports = { + StaleHeadError, assertCurrentHead, isClosedUnmerged, isOpenNonDraftAtHeadNow, +}; diff --git a/.github/pr-automation/fetch-pr-evidence.sh b/.github/pr-automation/fetch-pr-evidence.sh index 598f3e413..5a0e22eb0 100755 --- a/.github/pr-automation/fetch-pr-evidence.sh +++ b/.github/pr-automation/fetch-pr-evidence.sh @@ -8,22 +8,14 @@ # This is shared by every stage because the instruction-file removal below is a security boundary that duplicated scripts would eventually drift from. set -euo pipefail -if [ "$#" -ne 3 ]; then - echo "usage: fetch-pr-evidence.sh " >&2 +if [ "$#" -ne 2 ]; then + echo "usage: fetch-pr-evidence.sh " >&2 exit 1 fi head_sha="$1" base_sha="$2" -max_bytes="$3" -case "$max_bytes" in - 1048576) limit_mib=1 ;; - 4194304) limit_mib=4 ;; - *) - echo "error: invalid evidence diff limit" >&2 - exit 1 - ;; -esac +max_bytes=4194304 script_dir="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" export GIT_CONFIG_NOSYSTEM=1 export GIT_CONFIG_GLOBAL=/dev/null @@ -59,7 +51,7 @@ git -C pr-head diff --no-color --find-renames --unified=3 \ # Partial diffs cannot support complete classification or review. if [ "$(wc -c < pr-evidence/pull-request.diff)" -gt "$max_bytes" ]; then - reason="pull request diff exceeds the $limit_mib MiB evidence limit" + reason="pull request diff exceeds the 4 MiB evidence limit" printf '%s\n' "$reason" > pr-evidence/failure-reason.txt printf 'error: %s\n' "$reason" >&2 exit 1 diff --git a/.github/pr-automation/labels.json b/.github/pr-automation/labels.json index 3567fd9f2..c767a0c19 100644 --- a/.github/pr-automation/labels.json +++ b/.github/pr-automation/labels.json @@ -133,10 +133,5 @@ "name": "ai-reviewed/2", "color": "000000", "description": "Final automated review completed" - }, - { - "name": "ai-review/allow-oversized", - "color": "fbca04", - "description": "Raises the automated-review evidence diff limit from 1 MiB to 4 MiB" } ] diff --git a/.github/pr-automation/lifecycle.js b/.github/pr-automation/lifecycle.js new file mode 100644 index 000000000..e7a417e69 --- /dev/null +++ b/.github/pr-automation/lifecycle.js @@ -0,0 +1,122 @@ +"use strict"; + +const { readCheckRuns } = require("./check-runs"); +const { readLatestExactHeadCiRun } = require("./ci-state"); +const { canonicalRuns, ownerIsActive, parseLeaseMarker } = require("./automation-lease"); +const { parseCheckState } = require("./validate-classifier"); +const { trustedReviewOutcome } = require("./review-outcome"); +const { ACTOR_LABELS, labelsOf } = require("./resolve-state"); + +async function activeLease(github, owner, repo, run, headSha) { + const lease = parseLeaseMarker(run?.output?.summary); + return run?.status === "in_progress" && lease?.headSha === headSha && + await ownerIsActive(github, { ...lease, owner, repo }); +} + +function lifecycleActor(snapshot) { + if (snapshot.state !== "open" || snapshot.draft || snapshot.activeLease) return []; + const ci = snapshot.ci; + if (!ci || ci.status !== "completed") return []; + if (snapshot.canonicalAmbiguous) return []; + if (ci.conclusion === "failure") return ["needs-author-action"]; + if (ci.conclusion !== "success") return []; + if (snapshot.untrustedReviewSuccess) return null; + if (snapshot.labels.has("automation-failed")) return []; + if (snapshot.reviewOutcome === "findings") return ["needs-author-action"]; + if (snapshot.reviewOutcome === "no-findings") return ["needs-review"]; + if (snapshot.classificationValid && + (snapshot.labels.has("triage/legitimacy") || snapshot.labels.has("ai-reviewed/2"))) { + return ["needs-review"]; + } + return []; +} + +async function readLifecycleSnapshot({ github, owner, repo, prNumber, observedRun = null, ciRetry = {} }) { + const { data: pull } = await github.rest.pulls.get({ owner, repo, pull_number: prNumber }); + const labels = labelsOf((await github.rest.issues.get({ owner, repo, issue_number: prNumber })).data.labels); + const headSha = pull.head?.sha; + if (!/^[0-9a-f]{40}$/.test(headSha || "") || pull.state !== "open" || pull.draft) { + return { state: pull.state, draft: pull.draft === true, labels, headSha, activeLease: false }; + } + const [classificationRuns, reviewRuns, ci] = await Promise.all([ + readCheckRuns({ github, owner, repo, ref: headSha, checkName: "AI classification" }), + readCheckRuns({ github, owner, repo, ref: headSha, checkName: "AI automated review" }), + readLatestExactHeadCiRun({ github, owner, repo, expectedSha: headSha, observedRun, ...ciRetry }), + ]); + const classifications = canonicalRuns(classificationRuns || [], { kind: "classification", headSha }); + const reviews = canonicalRuns(reviewRuns || [], { kind: "review", headSha }); + const latestClassification = classifications?.[0] ?? null; + const latestReview = reviews?.[0] ?? null; + const [classificationLease, reviewLease] = await Promise.all([ + activeLease(github, owner, repo, latestClassification, headSha), + activeLease(github, owner, repo, latestReview, headSha), + ]); + const successfulReviews = reviews?.filter((run) => run.conclusion === "success") ?? []; + const newestSuccessfulReview = successfulReviews[0] ?? null; + const reviewOutcome = trustedReviewOutcome(newestSuccessfulReview, headSha); + return { + state: pull.state, + draft: pull.draft === true, + labels, + headSha, + ci, + activeLease: classificationLease || reviewLease, + canonicalAmbiguous: classifications === null || reviews === null, + classificationValid: latestClassification?.conclusion === "success" && + ["Classification complete", "Automation stopped"].includes(latestClassification.output?.title) && + parseCheckState(latestClassification.output?.summary) !== null, + reviewOutcome, + untrustedReviewSuccess: newestSuccessfulReview !== null && reviewOutcome === null, + }; +} + +async function reconcileLifecycle({ github, owner, repo, prNumber, observedRun = null, ciRetry = {} }) { + let changed = false; + let actor = []; + // One bounded verification pass converges a transition that races a write without creating a loop. + for (let pass = 0; pass < 2; pass += 1) { + const snapshot = await readLifecycleSnapshot({ + github, owner, repo, prNumber, observedRun, ciRetry, + }); + const desired = lifecycleActor(snapshot); + if (desired === null) { + return { ok: true, changed, reason: "review outcome receipt unavailable" }; + } + actor = desired; + const remove = ACTOR_LABELS.filter((label) => snapshot.labels.has(label) && !desired.includes(label)); + const add = desired.filter((label) => !snapshot.labels.has(label)); + if (remove.length === 0 && add.length === 0) break; + + for (const label of remove) { + const current = await readLifecycleSnapshot({ + github, owner, repo, prNumber, observedRun, ciRetry, + }); + const currentDesired = lifecycleActor(current); + if (currentDesired === null) return { ok: true, changed, reason: "review outcome receipt unavailable" }; + if (!current.labels.has(label) || currentDesired.includes(label)) continue; + try { + await github.rest.issues.removeLabel({ owner, repo, issue_number: prNumber, name: label }); + changed = true; + } catch (error) { + if (error?.status !== 404) throw error; + } + } + + const current = await readLifecycleSnapshot({ + github, owner, repo, prNumber, observedRun, ciRetry, + }); + const currentDesired = lifecycleActor(current); + if (currentDesired === null) return { ok: true, changed, reason: "review outcome receipt unavailable" }; + const additions = currentDesired.filter((label) => !current.labels.has(label)); + if (additions.length > 0) { + await github.rest.issues.addLabels({ owner, repo, issue_number: prNumber, labels: additions }); + changed = true; + } + actor = currentDesired; + } + return { ok: true, changed, actor }; +} + +module.exports = { + lifecycleActor, readLifecycleSnapshot, reconcileLifecycle, +}; diff --git a/.github/pr-automation/resolve-pr.js b/.github/pr-automation/resolve-pr.js index 88216de1a..d33e4f512 100644 --- a/.github/pr-automation/resolve-pr.js +++ b/.github/pr-automation/resolve-pr.js @@ -1,11 +1,18 @@ "use strict"; -const { OVERSIZED_REVIEW_LABEL } = require("./resolve-state"); - const SHA = /^[0-9a-f]{40}$/; -function noResult(reason, route = "unknown") { - return { ok: false, route, reason }; +function noResult(reason, route = "unknown", pr = null, observedCiRun = null) { + return { + ok: false, route, reason, + ...(observedCiRun ? { observedCiRun } : {}), + ...(pr ? { + prNumber: pr.number, + headSha: pr.head?.sha, + baseSha: pr.base?.sha, + labels: (pr.labels || []).map((label) => typeof label === "string" ? label : label.name).filter(Boolean), + } : {}), + }; } function routeFor(context) { @@ -27,9 +34,9 @@ function inputFlag(value) { return ["true", true].includes(value); } -async function getOpenAtHead(github, owner, repo, number, requiredSha) { +async function getPull(github, owner, repo, number) { const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: number }); - return pr.state === "open" && SHA.test(pr.head?.sha || "") && (!requiredSha || pr.head.sha === requiredSha) ? pr : null; + return SHA.test(pr.head?.sha || "") ? pr : null; } async function workflowRunPullRequests(github, owner, repo, workflowRun) { @@ -38,8 +45,8 @@ async function workflowRunPullRequests(github, owner, repo, workflowRun) { const candidates = new Set((workflowRun.pull_requests || []).map((pr) => positiveNumber(pr.number)).filter(Boolean)); const matches = []; for (const number of candidates) { - const pr = await getOpenAtHead(github, owner, repo, number, wantedSha); - if (pr) matches.push(pr); + const pr = await getPull(github, owner, repo, number); + if (pr?.head.sha === wantedSha) matches.push(pr); } if (matches.length > 0) return matches; @@ -48,8 +55,8 @@ async function workflowRunPullRequests(github, owner, repo, workflowRun) { })) { for (const candidate of response.data) { if (candidate.head?.sha !== wantedSha) continue; - const pr = await getOpenAtHead(github, owner, repo, candidate.number, wantedSha); - if (pr) matches.push(pr); + const pr = await getPull(github, owner, repo, candidate.number); + if (pr?.head.sha === wantedSha) matches.push(pr); } } return matches; @@ -61,31 +68,33 @@ async function resolvePr({ github, context, inputs = {} }) { const force = route === "dispatch" && inputFlag(inputs.force ?? context.payload.inputs?.force); const dispatchReview = route === "dispatch" && inputFlag(inputs.review ?? context.payload.inputs?.review); - const oversizedReviewRequested = route === "classification" && - context.payload.action === "labeled" && context.payload.label?.name === OVERSIZED_REVIEW_LABEL; let pr; let observedCiRun = null; try { if (route === "classification") { - // State writes also emit `labeled` events, so only the explicit maintainer opt-in may start - // automation through label events. Removing the opt-in must restart classification so the - // shared PR concurrency group cancels any queued or running use of the larger evidence cap. - if (["labeled", "unlabeled"].includes(context.payload.action) && - context.payload.label?.name !== OVERSIZED_REVIEW_LABEL) { - return noResult("unrelated pull request label", route); - } const number = positiveNumber(context.payload.pull_request?.number); if (!number) return noResult("missing pull request number", route); - pr = await getOpenAtHead(github, owner, repo, number); + pr = await getPull(github, owner, repo, number); } else if (route === "dispatch") { const number = positiveNumber(inputs.prNumber ?? context.payload.inputs?.["pr-number"]); if (!number) return noResult("invalid dispatch pull request number", route); - pr = await getOpenAtHead(github, owner, repo, number); + pr = await getPull(github, owner, repo, number); } else if (route === "ci") { const source = context.payload.workflow_run; - const matches = await workflowRunPullRequests(github, owner, repo, source); - if (matches.length !== 1) return noResult("workflow run did not resolve exactly one current PR", route); - pr = matches[0]; + const eventNumbers = new Set((source?.pull_requests || []) + .map((candidate) => positiveNumber(candidate.number)) + .filter(Boolean)); + if (eventNumbers.size === 1) { + pr = await getPull(github, owner, repo, [...eventNumbers][0]); + if (pr && pr.head.sha !== source?.head_sha) { + return noResult("workflow run head is stale", route, pr); + } + } + if (!pr) { + const matches = await workflowRunPullRequests(github, owner, repo, source); + if (matches.length !== 1) return noResult("workflow run did not resolve exactly one current PR", route); + pr = matches[0]; + } // The completed run is authoritative for its own generation while the run listing catches up. observedCiRun = { id: source.id, run_attempt: source.run_attempt, head_sha: source.head_sha, @@ -98,31 +107,36 @@ async function resolvePr({ github, context, inputs = {} }) { const number = positiveNumber(context.payload.client_payload?.pr_number); const headSha = context.payload.client_payload?.head_sha; if (!number || !SHA.test(headSha || "")) return noResult("invalid classification dispatch", route); - pr = await getOpenAtHead(github, owner, repo, number, headSha); + pr = await getPull(github, owner, repo, number); + if (pr && pr.head.sha !== headSha) return noResult("classification dispatch head is stale", route, pr); } else { return noResult("unsupported event", route); } } catch { return noResult("GitHub API unavailable", route); } - if (!pr) return noResult("pull request is closed or stale", route); - if (pr.draft && !force) return noResult("pull request is draft", route); + if (!pr) return noResult("pull request is stale", route); + if (pr.state !== "open") return noResult("pull request is closed", route, pr, observedCiRun); + if (pr.draft && !force) return noResult("pull request is draft", route, pr, observedCiRun); // Dependabot owns dependency and language labels, while devolutionsbot opens release-plz PRs. // This automation must not mutate either kind of automated pull request. const authorLogin = pr.user?.login || ""; const authorIsBot = pr.user?.type === "Bot" || /\[bot\]$/i.test(authorLogin) || authorLogin.toLowerCase() === "devolutionsbot"; - if (authorIsBot && !force) return noResult("bot-authored pull request", route); + if (authorIsBot && !force) { + return noResult("bot-authored pull request", route, pr, observedCiRun); + } const labels = (pr.labels || []) .map((label) => typeof label === "string" ? label : label.name) .filter(Boolean); - const reviewRequested = oversizedReviewRequested || dispatchReview; + const unrelatedLabel = route === "classification" && ["labeled", "unlabeled"].includes(context.payload.action); + if (unrelatedLabel) return noResult("unrelated pull request label", route, pr); + const reviewRequested = dispatchReview; const classificationRequested = route === "classification" || (route === "dispatch" && !dispatchReview); return { ok: true, route, prNumber: pr.number, headSha: pr.head.sha, baseSha: pr.base.sha, labels, - evidenceMaxBytes: labels.includes(OVERSIZED_REVIEW_LABEL) ? 4 * 1024 * 1024 : 1024 * 1024, author: { nodeId: pr.user?.node_id || null, login: pr.user?.login || null, type: pr.user?.type || null, association: pr.author_association || null, diff --git a/.github/pr-automation/resolve-state.js b/.github/pr-automation/resolve-state.js index a14cd4362..013680820 100644 --- a/.github/pr-automation/resolve-state.js +++ b/.github/pr-automation/resolve-state.js @@ -12,15 +12,13 @@ const ACTOR_LABELS = ["needs-review", "needs-author-action"]; const FAILURE_LABEL = "automation-failed"; const LEGITIMACY_LABEL = "triage/legitimacy"; const OVERLAP_LABEL = "triage/overlap"; -const OVERSIZED_REVIEW_LABEL = "ai-review/allow-oversized"; const LEGITIMACY_MARKER_PREFIX = ""; -const OVERSIZED_MARKER = ""; const LEGACY_XL_MARKER = ""; const FORK_QUOTA_MARKER = ""; const GLOBAL_QUOTA_MARKER = ""; const EVIDENCE_LIMIT_MARKER = ""; -const EVIDENCE_LIMIT_REASON = /^pull request diff exceeds the (1|4) MiB evidence limit$/; +const EVIDENCE_LIMIT_REASON = /^pull request diff exceeds the 4 MiB evidence limit$/; function labelsOf(labels) { return new Set((labels || []).map((label) => typeof label === "string" ? label : label?.name).filter(Boolean)); @@ -28,24 +26,18 @@ function labelsOf(labels) { function reviewCount(labels) { const present = labels instanceof Set ? labels : labelsOf(labels); - return present.has("ai-reviewed/2") ? "ai-reviewed/2" - : present.has("ai-reviewed/1") ? "ai-reviewed/1" - : null; + const first = present.has("ai-reviewed/1"); + const second = present.has("ai-reviewed/2"); + if (first && second) return undefined; + return second ? "ai-reviewed/2" : first ? "ai-reviewed/1" : null; } -function actorFailureLabelSets(actor = [], failure = []) { - return [ - { owned: ACTOR_LABELS, desired: actor }, - { owned: [FAILURE_LABEL], desired: failure }, - ]; +function failureLabelSets(failure = []) { + return [{ owned: [FAILURE_LABEL], desired: failure }]; } -function preservedActorFailureLabelSets(labels) { - const actors = ACTOR_LABELS.filter((label) => labels.has(label)); - return actorFailureLabelSets( - actors.length === 1 ? actors : [], - labels.has(FAILURE_LABEL) ? [FAILURE_LABEL] : [], - ); +function preservedFailureLabelSets(labels) { + return failureLabelSets(labels.has(FAILURE_LABEL) ? [FAILURE_LABEL] : []); } function boundStatus(value, expectedSha, allowed) { @@ -59,9 +51,8 @@ function quotaComment(rateLimit) { } function evidenceLimitComment(reason) { - const match = typeof reason === "string" ? EVIDENCE_LIMIT_REASON.exec(reason) : null; - return match - ? { kind: "evidence-limit", marker: EVIDENCE_LIMIT_MARKER, limitMiB: Number(match[1]) } + return typeof reason === "string" && EVIDENCE_LIMIT_REASON.test(reason) + ? { kind: "evidence-limit", marker: EVIDENCE_LIMIT_MARKER } : null; } @@ -97,7 +88,7 @@ function failedClassification(expectedSha, deterministic, reason, rateLimit, sem labelSets: [ ...deterministicLabelSets(deterministic), { owned: RISK, desired: [semverStatus === "suspected" ? "risk/high" : "risk/unknown"] }, - ...actorFailureLabelSets([], [FAILURE_LABEL]), + ...failureLabelSets([FAILURE_LABEL]), ...(semverStatus === "suspected" ? [{ owned: ["breaking-change"], desired: ["breaking-change"] }] : []), @@ -185,7 +176,7 @@ function resolveClassificationState({ { owned: ["scope/cross-cutting"], desired: model.cross_cutting ? ["scope/cross-cutting"] : [] }, ...optional.map(([label, enabled]) => ({ owned: [label], desired: enabled ? [label] : [] })), { owned: ["breaking-change"], desired: breaking ? ["breaking-change"] : [] }, - ...(forced ? preservedActorFailureLabelSets(existing) : actorFailureLabelSets()), + ...(forced ? preservedFailureLabelSets(existing) : failureLabelSets()), ]; const legitimacyStopped = model.likely_non_legitimate; const addLabels = [ @@ -212,7 +203,6 @@ function resolveClassificationState({ EVIDENCE_LIMIT_MARKER, FORK_QUOTA_MARKER, GLOBAL_QUOTA_MARKER, - OVERSIZED_MARKER, LEGACY_XL_MARKER, ], check: { @@ -258,8 +248,8 @@ function resolveReviewState({ ? { ciRunId: ciGeneration.id, ciRunAttempt: ciGeneration.attempt } : {}; const state = (reason, { - failed = false, report = false, actor = [], failure = [], - blocked = false, labelSets = actorFailureLabelSets(actor, failure), + failed = false, report = false, failure = [], + blocked = false, labelSets = failureLabelSets(failure), } = {}) => { const comments = [ forced ? null : quotaComment(rateLimit), @@ -284,38 +274,36 @@ function resolveReviewState({ } } : {}), }; }; - const blocked = (reason, { clearActors = false } = {}) => state(reason, { - blocked: true, - labelSets: clearActors ? [{ owned: ACTOR_LABELS, desired: [] }] : [], - }); + const blocked = (reason) => state(reason, { blocked: true, labelSets: [] }); const fail = (reason) => state(reason, { - failed: true, report: true, actor: [], failure: [FAILURE_LABEL], + failed: true, report: true, failure: [FAILURE_LABEL], }); if (typeof expectedSha !== "string") return { ok: false, reason: "missing expected SHA" }; + const currentReviewCount = reviewCount(existing); + if (currentReviewCount === undefined) return blocked("review count is ambiguous"); if (forced) { if (gate?.force !== true || gate.head_sha !== expectedSha) return fail("forced review gate unavailable"); const classification = validateReviewGate(gate, expectedSha); if (!classification.ok) return fail(classification.reason); + if (!Number.isSafeInteger(gate.classificationId) || gate.classificationId <= 0) { + return fail("forced classification identity unavailable"); + } if (typeof reviewMarkerId !== "string" || !/^[1-9]\d{0,19}$/.test(reviewMarkerId)) { return fail("forced review marker unavailable"); } } else { if (!gate || gate.head_sha !== expectedSha) return blocked("review gate unavailable"); - if (gate.ciGreen !== true) return blocked("CI has not succeeded", { clearActors: true }); + if (gate.ciGreen !== true) return blocked("CI has not succeeded"); if (gate.classificationValid === true && (existing.has("ai-reviewed/2") || gate.legitimacyStopped === true || existing.has(LEGITIMACY_LABEL))) { return { - ...state("review is handed to a human", { - actor: ["needs-review"], failure: [], - }), + ...state("review is handed to a human"), handoff: existing.has("ai-reviewed/2") ? "terminal" : "legitimacy", ...ciState, }; } - if (rateLimit && rateLimit.status !== "allowed") return blocked("fork LLM quota unavailable", { - clearActors: true, - }); + if (rateLimit && rateLimit.status !== "allowed") return blocked("fork LLM quota unavailable"); if (typeof gate.ok !== "boolean" || gate.reason) { const reason = gate?.reason ? `review gate unavailable: ${gate.reason}` : "review gate unavailable"; return blocked(reason); @@ -353,7 +341,6 @@ function resolveReviewState({ if (!reviewerResult?.ok || reviewerResult.value?.head_sha !== expectedSha) { return fail(reviewerReason || reviewerResult?.reason || "reviewer unavailable"); } - const currentReviewCount = reviewCount(existing); const nextCount = currentReviewCount === "ai-reviewed/2" ? "ai-reviewed/2" : currentReviewCount === "ai-reviewed/1" ? "ai-reviewed/2" : "ai-reviewed/1"; @@ -365,19 +352,30 @@ function resolveReviewState({ ok: true, mode: "review", expectedSha, labelSets: [ { owned: AI_COUNTS, desired: [nextCount] }, - ...actorFailureLabelSets( - hasFindings ? ["needs-author-action"] : ["needs-review"], - [], - ), + ...failureLabelSets(), ], comments: [{ kind: "review", marker: reviewMarker, review: reviewerResult.value, reducedCoverage: Array.isArray(reducedCoverage) ? reducedCoverage : [], }], removeCommentMarkers: [EVIDENCE_LIMIT_MARKER, FORK_QUOTA_MARKER, GLOBAL_QUOTA_MARKER], - check: { name: "AI automated review", externalId: expectedSha }, + check: { + name: "AI automated review", + externalId: expectedSha, + outcome: hasFindings ? "findings" : "no-findings", + }, + outcome: hasFindings ? "findings" : "no-findings", expectedReviewCount, + nextReviewCount: nextCount, forced, + admittedGate: { + classificationId: gate.classificationId, + ciRunId: ciGeneration?.id ?? null, + ciRunAttempt: ciGeneration?.attempt ?? null, + labels: gate.labels || [], + policyEligible: gate.policyEligible, + legitimacyStopped: gate.legitimacyStopped, + }, ...(!forced ? ciState : {}), protocolRelated: gate.protocolRelated === true, }; @@ -394,8 +392,7 @@ function reviewOutcome({ reportStatus, state, reducedCoverage = [] } = {}) { module.exports = { ACTOR_LABELS, AI_COUNTS, EVIDENCE_LIMIT_MARKER, FAILURE_LABEL, FORK_QUOTA_MARKER, GLOBAL_QUOTA_MARKER, LEGACY_XL_MARKER, LEGITIMACY_LABEL, - LEGITIMACY_MARKER_PREFIX, OVERLAP_LABEL, OVERLAP_MARKER, OVERSIZED_REVIEW_LABEL, RISK, - OVERSIZED_MARKER, + LEGITIMACY_MARKER_PREFIX, OVERLAP_LABEL, OVERLAP_MARKER, RISK, labelsOf, contributorEligibility, resolveClassificationState, resolveReviewState, reviewCount, reviewOutcome, reviewPolicyEligible, }; diff --git a/.github/pr-automation/review-outcome.js b/.github/pr-automation/review-outcome.js new file mode 100644 index 000000000..eef740573 --- /dev/null +++ b/.github/pr-automation/review-outcome.js @@ -0,0 +1,40 @@ +"use strict"; + +const { exactKeys } = require("./validation"); + +const APP = "github-actions"; +const SCHEMA_VERSION = "review-outcome-v1"; +const MARKER = "ironrdp-pr-automation-review-outcome:"; + +function encodeReviewOutcome({ headSha, outcome } = {}) { + if (!/^[0-9a-f]{40}$/.test(headSha || "") || !["findings", "no-findings"].includes(outcome)) { + throw new Error("invalid review outcome"); + } + return `${MARKER} ${JSON.stringify({ + schema_version: SCHEMA_VERSION, + head_sha: headSha, + outcome, + })}`; +} + +function parseReviewOutcome(text, expectedSha) { + if (typeof text !== "string" || Buffer.byteLength(text, "utf8") > 4096) return null; + const lines = text.split(/\r?\n/).map((entry) => entry.trim()).filter((entry) => entry.startsWith(MARKER)); + if (lines.length !== 1) return null; + let parsed; + try { parsed = JSON.parse(lines[0].slice(MARKER.length)); } catch { return null; } + if (!exactKeys(parsed, ["schema_version", "head_sha", "outcome"]) || + parsed.schema_version !== SCHEMA_VERSION || parsed.head_sha !== expectedSha || + !["findings", "no-findings"].includes(parsed.outcome)) return null; + return parsed.outcome; +} + +function trustedReviewOutcome(run, expectedSha) { + if (run?.app?.slug !== APP || run.head_sha !== expectedSha || + run.external_id !== expectedSha || run.conclusion !== "success") return null; + return parseReviewOutcome(run.output?.summary, expectedSha); +} + +module.exports = { + MARKER, SCHEMA_VERSION, encodeReviewOutcome, parseReviewOutcome, trustedReviewOutcome, +}; diff --git a/.github/pr-automation/write-state.js b/.github/pr-automation/write-state.js index 62695a13c..4be7c30a9 100644 --- a/.github/pr-automation/write-state.js +++ b/.github/pr-automation/write-state.js @@ -8,15 +8,29 @@ const { reducedCoverageText, reviewBody, } = require("./review-render"); const { reviewPolicyEligible } = require("./routing"); -const { ownsActiveLease, parseLeaseMarker } = require("./automation-lease"); -const { assertCurrentHead } = require("./current-head"); +const { + hasCurrentClassification, ownsActiveLease, parseLeaseMarker, recheckAutomaticReview, +} = require("./automation-lease"); +const { + assertCurrentHead, isClosedUnmerged, isOpenNonDraftAtHeadNow, +} = require("./current-head"); const { matchesGeneration, readLatestExactHeadCiRun } = require("./ci-state"); -const { ACTOR_LABELS, reviewCount } = require("./resolve-state"); +const { ACTOR_LABELS, AI_COUNTS, reviewCount } = require("./resolve-state"); +const { encodeReviewOutcome } = require("./review-outcome"); class StalePolicyError extends Error { constructor() { super("pull request review policy changed"); this.name = "StalePolicyError"; } } +class ClosedReviewRejectedError extends Error { + constructor() { super("GitHub rejected review publication for a closed pull request"); } +} + +async function isClosedUnmergedAtHead(github, owner, repo, prNumber, expectedSha) { + const { data: pull } = await github.rest.pulls.get({ owner, repo, pull_number: prNumber }); + return isClosedUnmerged(pull) && pull.head?.sha === expectedSha; +} + async function issueLabels(github, owner, repo, prNumber) { const { data } = await github.rest.issues.get({ owner, repo, issue_number: prNumber }); return new Set(data.labels.map((label) => typeof label === "string" ? label : label.name).filter(Boolean)); @@ -41,16 +55,13 @@ function markerBody(comment, owner, repo) { return `${comment.marker}\n\nAutomated classification and review capacity for fork pull requests has reached its daily UTC limit.\n\nSee the [automation policy](https://github.com/${owner}/${repo}/blob/master/.github/PR_AUTOMATION.md). Automation remains blocked until capacity is available and it is retried.`; } if (comment.kind === "evidence-limit") { - const guidance = comment.limitMiB === 1 - ? "A maintainer can add `ai-review/allow-oversized` to retry with the runtime maximum of 4 MiB. Otherwise, split the change into focused pull requests or reduce generated content." - : "The 4 MiB limit is the model runtime maximum. Please split the change into focused pull requests or reduce generated content."; - return `${comment.marker}\n\nAutomated model analysis stopped because this pull request's diff exceeds the ${comment.limitMiB} MiB evidence limit. No model was invoked with partial evidence.\n\n${guidance} Automation remains blocked until retry or repair.`; + return `${comment.marker}\n\nAutomated model analysis stopped because this pull request's diff exceeds the 4 MiB evidence limit. No model was invoked with partial evidence.\n\nPlease split the change into focused pull requests or reduce generated content. Automation remains blocked until retry or repair.`; } throw new Error("unsupported issue comment"); } async function upsertMarkedComment( - github, owner, repo, prNumber, expectedSha, botLogin, comment, canMutate, + github, owner, repo, prNumber, expectedSha, botLogin, comment, canMutate, allowClosedUnmerged = false, ) { if (!botLogin || typeof botLogin !== "string") throw new Error("botLogin is required for comment ownership"); const body = markerBody(comment, owner, repo); @@ -58,7 +69,9 @@ async function upsertMarkedComment( item.user?.login === botLogin && typeof item.body === "string" && item.body.includes(comment.marker)); if (existing?.body === body) return false; await issueLabels(github, owner, repo, prNumber); - await assertCurrentHead({ github, owner, repo, pullNumber: prNumber, expectedHeadSha: expectedSha }); + await assertCurrentHead({ + github, owner, repo, pullNumber: prNumber, expectedHeadSha: expectedSha, allowClosedUnmerged, + }); if (canMutate && !await canMutate()) return false; if (existing) { await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }); @@ -69,13 +82,15 @@ async function upsertMarkedComment( } async function deleteMarkedComment( - github, owner, repo, prNumber, expectedSha, botLogin, marker, canMutate, + github, owner, repo, prNumber, expectedSha, botLogin, marker, canMutate, allowClosedUnmerged = false, ) { if (!botLogin || typeof botLogin !== "string") throw new Error("botLogin is required for comment ownership"); const existing = (await comments(github, owner, repo, prNumber)).find((item) => item.user?.login === botLogin && typeof item.body === "string" && item.body.includes(marker)); if (!existing) return false; - await assertCurrentHead({ github, owner, repo, pullNumber: prNumber, expectedHeadSha: expectedSha }); + await assertCurrentHead({ + github, owner, repo, pullNumber: prNumber, expectedHeadSha: expectedSha, allowClosedUnmerged, + }); if (canMutate && !await canMutate()) return false; await github.rest.issues.deleteComment({ owner, repo, comment_id: existing.id }); return true; @@ -158,13 +173,23 @@ async function publishReview( await assertFreshCi(github, owner, repo, state, ciRetry); await assertCurrentHead({ github, owner, repo, pullNumber: prNumber, expectedHeadSha: state.expectedSha, + allowClosedUnmerged: state.allowClosedUnmerged === true, }); assertReviewPolicy(await issueLabels(github, owner, repo, prNumber), state); if (canMutate && !await canMutate()) return false; - await github.rest.pulls.createReview({ - owner, repo, pull_number: prNumber, commit_id: state.expectedSha, event: "COMMENT", - body, comments: inline, - }); + try { + await github.rest.pulls.createReview({ + owner, repo, pull_number: prNumber, commit_id: state.expectedSha, event: "COMMENT", + body, comments: inline, + }); + } catch (error) { + if (state.allowClosedUnmerged === true && [404, 409, 422].includes(error?.status) && + await isClosedUnmergedAtHead(github, owner, repo, prNumber, state.expectedSha) && + (!canMutate || await canMutate())) { + throw new ClosedReviewRejectedError(); + } + throw error; + } return true; } @@ -204,6 +229,7 @@ async function ensureCanonicalCheck({ if (beforeWrite) await beforeWrite(); await assertCurrentHead({ github, owner, repo, pullNumber: prNumber, expectedHeadSha: expectedSha, + allowClosedUnmerged: state.allowClosedUnmerged === true, }); if (canMutate && !await canMutate()) return false; const payload = { @@ -236,35 +262,44 @@ async function ensureReviewCheck( github, owner, repo, prNumber, expectedSha, check, state, canMutate, ciRetry, ) { const title = check.title ?? "Automated review complete"; - const summary = check.summary ?? "Validated automated review is bound to this commit."; + const summary = check.outcome + ? `${check.summary ?? "Validated automated review is bound to this commit."}\n\n${encodeReviewOutcome({ + headSha: expectedSha, outcome: check.outcome, + })}` + : check.summary ?? "Validated automated review is bound to this commit."; return ensureCanonicalCheck({ github, owner, repo, prNumber, expectedSha, check, state, title, summary, canMutate, beforeWrite: state.failed === true || state.blocked === true ? null : async () => { await assertFreshCi(github, owner, repo, state, ciRetry); - assertReviewPolicy(await issueLabels(github, owner, repo, prNumber), state); }, }); } -async function dispatchClassificationComplete(github, owner, repo, prNumber, expectedSha) { +async function dispatchClassificationComplete(github, owner, repo, prNumber, expectedSha, canMutate) { await assertCurrentHead({ github, owner, repo, pullNumber: prNumber, expectedHeadSha: expectedSha, }); + if (canMutate && !await canMutate()) return false; await github.rest.repos.createDispatchEvent({ owner, repo, event_type: "pr-automation-classified", client_payload: { pr_number: prNumber, head_sha: expectedSha }, }); + return true; } // Computes the whole label delta from a single read, so a state with two label sets plus additions // and removals costs one issue read instead of one per candidate label. -async function applyLabels(github, owner, repo, prNumber, state, currentLabels, canMutate) { +async function applyLabels( + github, owner, repo, prNumber, state, currentLabels, canMutate, skipReviewCount = false, +) { const current = currentLabels ?? await issueLabels(github, owner, repo, prNumber); const add = new Set(); const remove = new Set(); for (const { owned, desired } of state.labelSets || []) { const wanted = new Set(desired || []); - for (const label of new Set(owned || [])) (wanted.has(label) ? add : remove).add(label); + for (const label of new Set(owned || [])) { + if (!skipReviewCount || !AI_COUNTS.includes(label)) (wanted.has(label) ? add : remove).add(label); + } } for (const label of state.addLabels || []) add.add(label); for (const label of state.removeLabels || []) { add.delete(label); remove.add(label); } @@ -273,6 +308,7 @@ async function applyLabels(github, owner, repo, prNumber, state, currentLabels, if (additions.length === 0 && removals.length === 0) return false; await assertCurrentHead({ github, owner, repo, pullNumber: prNumber, expectedHeadSha: state.expectedSha, + allowClosedUnmerged: state.allowClosedUnmerged === true, }); // The actor labels are mutually exclusive. Remove a stale actor before adding its replacement so // a failed add leaves no actor selected rather than two conflicting actors. Other labels retain @@ -303,6 +339,28 @@ async function applyLabels(github, owner, repo, prNumber, state, currentLabels, return true; } +async function transitionReviewCount({ + github, owner, repo, prNumber, state, canTransition, +}) { + if (![null, ...AI_COUNTS].includes(state.expectedReviewCount) || + !AI_COUNTS.includes(state.nextReviewCount)) throw new StalePolicyError(); + if (!await canTransition("transition-old")) return false; + if (state.expectedReviewCount !== state.nextReviewCount) { + if (state.expectedReviewCount) { + await github.rest.issues.removeLabel({ + owner, repo, issue_number: prNumber, name: state.expectedReviewCount, + }); + } + // Removal first makes an interrupted /1 -> /2 transition undercount rather than overcount. + // The rare partial publication is intentionally left for manual repair. + if (!await canTransition("transition-empty")) return false; + await github.rest.issues.addLabels({ + owner, repo, issue_number: prNumber, labels: [state.nextReviewCount], + }); + } + return await canTransition("transition-next"); +} + async function completeNeutralCheck(github, owner, repo, checkRunId, marker, title) { await github.rest.checks.update({ owner, repo, check_run_id: checkRunId, status: "completed", conclusion: "neutral", @@ -324,15 +382,76 @@ async function writeState({ typeof state.expectedSha !== "string" || !Number.isSafeInteger(prNumber) || prNumber <= 0) { throw new Error("invalid normalized state"); } - const canMutate = async () => !state.lease || state.forced === true || await ownsActiveLease({ github, owner, repo, lease: state.lease }); + const canMutate = async () => !state.lease || state.forced === true || + await ownsActiveLease({ github, owner, repo, lease: state.lease }); + const automaticReviewContinuation = state.mode === "review" && state.forced !== true && + state.failed !== true && state.blocked !== true && state.lease !== undefined && state.lease !== null; + state.allowClosedUnmerged = automaticReviewContinuation; + const canPublishClassification = async () => { + if (!await canMutate()) return false; + if (state.forced === true) return true; + return await isOpenNonDraftAtHeadNow({ + github, owner, repo, pullNumber: prNumber, expectedHeadSha: state.expectedSha, + }); + }; + const hasForcedClassification = async () => await hasCurrentClassification({ + github, owner, repo, headSha: state.expectedSha, + classificationId: state.admittedGate?.classificationId, requireValid: true, + }); + const canPublishReview = async () => { + if (!await canMutate()) return false; + if (state.failed === true || state.blocked === true) return true; + if (state.forced === true) return await hasForcedClassification(); + // A normal automatic lease must carry the admission facts it is continuing from. + // Lease-less legacy/explicit paths remain open-only because they cannot enable closed continuation. + if (!state.admittedGate?.classificationId) return !state.lease; + return await recheckAutomaticReview({ + github, owner, repo, prNumber, headSha: state.expectedSha, + gate: state.admittedGate, lease: state.lease, allowDraft: true, + allowClosedUnmerged: state.allowClosedUnmerged === true, + }); + }; + const canPublishReviewPublication = async () => { + if (state.forced !== true) return await canPublishReview(); + if (!await canMutate()) return false; + return await hasForcedClassification(); + }; + const canPublishReviewReceipt = async () => { + if (!await canMutate()) return false; + if (state.failed === true || state.blocked === true) return true; + if (state.forced === true) return await canPublishReviewPublication(); + if (!state.admittedGate?.classificationId || !state.nextReviewCount) return !state.lease; + return await recheckAutomaticReview({ + github, owner, repo, prNumber, headSha: state.expectedSha, + gate: { ...state.admittedGate, nextCount: state.nextReviewCount }, lease: state.lease, + phase: "receipt", allowDraft: true, allowClosedUnmerged: state.allowClosedUnmerged === true, + }); + }; + const canCleanupReview = async () => { + if (state.failed === true || state.blocked === true) return true; + if (state.forced === true) return await canPublishReviewPublication(); + if (!state.admittedGate?.classificationId || !state.nextReviewCount) return !state.lease; + return await recheckAutomaticReview({ + github, owner, repo, prNumber, headSha: state.expectedSha, + gate: { ...state.admittedGate, nextCount: state.nextReviewCount }, lease: state.lease, + phase: "cleanup", allowDraft: true, allowClosedUnmerged: state.allowClosedUnmerged === true, + }); + }; + const canPublish = state.mode === "classification" ? canPublishClassification : canPublishReview; if (state.superseded === true) { await completeLeaseNeutral( github, owner, repo, state, canMutate, "Automation superseded"); return { ok: true, superseded: true }; } - await assertCurrentHead({ + const pull = await assertCurrentHead({ github, owner, repo, pullNumber: prNumber, expectedHeadSha: state.expectedSha, + allowClosedUnmerged: automaticReviewContinuation, }); + if (state.forced !== true && pull.draft && + !(state.mode === "review" && state.failed !== true && state.blocked !== true)) { + await completeLeaseNeutral(github, owner, repo, state, canMutate, "Automation superseded"); + return { ok: true, superseded: true }; + } const check = state.check; let existingCheck = null; let successfulCheck = null; @@ -366,13 +485,24 @@ async function writeState({ } } } - if (!await canMutate()) return { ok: true, superseded: true }; + if (!await canPublish()) return { ok: true, superseded: true }; if (state.mode === "review") { const comments = state.comments || []; for (const comment of comments.filter((comment) => comment.kind === "review")) { - if (!await canMutate()) return { ok: true, superseded: true }; - await publishReview( - github, owner, repo, prNumber, state, botLogin, comment, canMutate, ciRetry); + if (!await canPublishReviewPublication()) return { ok: true, superseded: true }; + try { + const published = await publishReview( + github, owner, repo, prNumber, state, botLogin, comment, canPublishReviewPublication, ciRetry); + if (!published && automaticReviewContinuation) { + await completeLeaseNeutral( + github, owner, repo, state, canMutate, "Review publication requires manual repair"); + return { ok: true, superseded: true }; + } + } catch (error) { + if (!(error instanceof ClosedReviewRejectedError)) throw error; + await completeLeaseNeutral(github, owner, repo, state, canMutate, "Review publication unavailable"); + return { ok: true, superseded: true }; + } } const latestLabels = state.failed === true || state.blocked === true ? undefined @@ -381,42 +511,75 @@ async function writeState({ await assertFreshCi(github, owner, repo, state, ciRetry); assertReviewPolicy(latestLabels, state); } - if (!await canMutate()) return { ok: true, superseded: true }; - await applyLabels(github, owner, repo, prNumber, state, latestLabels, canMutate); - for (const comment of comments.filter((comment) => comment.kind !== "review")) { - if (!await canMutate()) return { ok: true, superseded: true }; - await upsertMarkedComment( - github, owner, repo, prNumber, state.expectedSha, botLogin, comment, canMutate); - } - for (const marker of new Set(state.removeCommentMarkers || [])) { - if (!await canMutate()) return { ok: true, superseded: true }; - await deleteMarkedComment( - github, owner, repo, prNumber, state.expectedSha, botLogin, marker, canMutate); + if (!await canPublishReview()) return { ok: true, superseded: true }; + if (state.failed !== true && state.blocked !== true && state.nextReviewCount && + (state.forced === true || automaticReviewContinuation)) { + const canTransition = async (phase) => { + if (state.forced === true) { + await assertCurrentHead({ + github, owner, repo, pullNumber: prNumber, expectedHeadSha: state.expectedSha, + }); + if (!await canPublishReviewPublication()) return false; + const count = reviewCount(await issueLabels(github, owner, repo, prNumber)); + const expected = phase === "transition-old" ? state.expectedReviewCount + : phase === "transition-empty" ? null + : state.nextReviewCount; + return count !== undefined && count === expected; + } + return await recheckAutomaticReview({ + github, owner, repo, prNumber, headSha: state.expectedSha, + gate: { ...state.admittedGate, nextCount: state.nextReviewCount }, + lease: state.lease, phase, allowDraft: true, + allowClosedUnmerged: state.allowClosedUnmerged === true, + }); + }; + if (!await transitionReviewCount({ + github, owner, repo, prNumber, state, canTransition, + })) return { ok: true, superseded: true }; } + await applyLabels( + github, owner, repo, prNumber, state, latestLabels, canPublishReview, + state.nextReviewCount !== undefined && (state.forced === true || automaticReviewContinuation), + ); if (state.check) { - if (!await canMutate()) return { ok: true, superseded: true }; + if (!await canPublishReviewReceipt()) return { ok: true, superseded: true }; await ensureReviewCheck( - github, owner, repo, prNumber, state.expectedSha, state.check, state, canMutate, ciRetry); + github, owner, repo, prNumber, state.expectedSha, state.check, state, canPublishReviewReceipt, ciRetry); } else if (state.lease && !await completeLeaseNeutral( github, owner, repo, state, canMutate, "Automation blocked", )) { return { ok: true, superseded: true }; } + // Marker cleanup is nonessential and therefore follows the review/count/receipt sequence. + for (const comment of comments.filter((comment) => comment.kind !== "review")) { + if (!await canCleanupReview()) return { ok: true, superseded: true }; + await upsertMarkedComment( + github, owner, repo, prNumber, state.expectedSha, botLogin, comment, canCleanupReview, + state.allowClosedUnmerged, + ); + } + for (const marker of new Set(state.removeCommentMarkers || [])) { + if (!await canCleanupReview()) return { ok: true, superseded: true }; + await deleteMarkedComment( + github, owner, repo, prNumber, state.expectedSha, botLogin, marker, canCleanupReview, + state.allowClosedUnmerged, + ); + } } else { - if (!await canMutate()) return { ok: true, superseded: true }; - await applyLabels(github, owner, repo, prNumber, state, undefined, canMutate); + if (!await canPublishClassification()) return { ok: true, superseded: true }; + await applyLabels(github, owner, repo, prNumber, state, undefined, canPublishClassification); for (const comment of state.comments || []) await upsertMarkedComment( - github, owner, repo, prNumber, state.expectedSha, botLogin, comment, canMutate); + github, owner, repo, prNumber, state.expectedSha, botLogin, comment, canPublishClassification); for (const comment of state.auditComments || []) await upsertMarkedComment( - github, owner, repo, prNumber, state.expectedSha, botLogin, comment, canMutate); + github, owner, repo, prNumber, state.expectedSha, botLogin, comment, canPublishClassification); for (const marker of new Set(state.removeCommentMarkers || [])) { await deleteMarkedComment( - github, owner, repo, prNumber, state.expectedSha, botLogin, marker, canMutate); + github, owner, repo, prNumber, state.expectedSha, botLogin, marker, canPublishClassification); } if (state.check) { - if (!await canMutate()) return { ok: true, superseded: true }; + if (!await canPublishClassification()) return { ok: true, superseded: true }; const changed = await ensureClassificationCheck( - github, owner, repo, prNumber, state.expectedSha, state.check, state, canMutate); + github, owner, repo, prNumber, state.expectedSha, state.check, state, canPublishClassification); const shouldDispatch = ["Classification complete", "Automation stopped"].includes( state.check.title, ) && state.dispatchReview !== false && (state.lease @@ -426,7 +589,7 @@ async function writeState({ (changed || reviewRequested || state.check.title === "Automation stopped")); if (shouldDispatch) { await dispatchClassificationComplete( - github, owner, repo, prNumber, state.expectedSha, + github, owner, repo, prNumber, state.expectedSha, canPublishClassification, ); } } diff --git a/.github/workflows/pr-automation.intent.md b/.github/workflows/pr-automation.intent.md index 02c10ec2c..45379e55e 100644 --- a/.github/workflows/pr-automation.intent.md +++ b/.github/workflows/pr-automation.intent.md @@ -77,18 +77,19 @@ Keep detailed failure reasons in the workflow summary only. Classify every non-draft, human-authored pull request that passes the integrity and capacity gates. Run automated review after the latest CI generation succeeds for the exact classified head. Run the second review after a later push reaches green exact-head CI. -At `ai-reviewed/2`, the review pipeline stops and the review route hands a green pull request to `needs-review`. +At `ai-reviewed/2`, the review pipeline stops and lifecycle reconciliation hands a green pull request to `needs-review`. `needs-review` means a human reviewer is the next actor, and `needs-author-action` means the author is the next actor. -They are automation-owned and mutually exclusive. +Lifecycle reconciliation exclusively owns these mutually exclusive actor labels. An attempted exact-head classification or eligible review failure applies only `automation-failed`. -Successful normal classification can clear stale actor and failure labels before it dispatches review. -An automated review with findings applies `needs-author-action`, and one without findings applies `needs-review`. -Valid classification plus green CI hands legitimacy triage to `needs-review` without a model call. -Blocked and duplicate review routes preserve or clear actor state through their resolved state without creating a failure label. +Every successful app-owned exact-head review check persists a bounded versioned `findings` or `no-findings` receipt. +Reconciliation trusts only a newest canonical successful check with the expected app, SHA, external ID, conclusion, schema, and exact receipt keys. +Existing successful checks without that receipt fail closed before lifecycle clearing. +Successful review outcomes, clean terminal handoffs, and legitimacy stops become actor labels only through a fresh reconciliation snapshot. +Closed and draft pull requests, active leases, and missing, pending, or nonstandard terminal CI conclusions select neither actor. +Exact-head CI failure selects `needs-author-action` independently of `automation-failed`. The final writer accepts successful non-forced review and handoff state only when the stored CI generation is still the latest green exact-head CI generation. -The CI completion that triggers review is authoritative for its own generation; a lagging run listing is not staleness and must not silently skip an eligible review, but a newer CI generation still wins. -Prompt lifecycle clearing and convergence are deferred to the supersession and reconciliation follow-up. +The CI event is authoritative for its exact-head generation while listings catch up, and a newer listed exact-head generation always wins. Every non-bot author is eligible immediately; there is no prior-merge requirement. @@ -101,16 +102,23 @@ Fork pull requests share a quota of 50 per UTC day. Exclude `OWNER` and `MEMBER` pull requests from enforcement and counting, and preserve the same-repository exemption. Keep `size/XXL` informational. -Use a 1 MiB evidence diff limit by default and a 4 MiB limit when `ai-review/allow-oversized` is present. -Adding that label must retry classification. -Evidence above the applicable limit fails closed without partial model input. +Use a 4 MiB evidence diff limit. +Evidence above that limit fails closed without partial model input. Normal classification-to-review dispatch is edge-triggered and occurs only when the persisted SHA-bound classification check changes. -Adding `ai-review/allow-oversized` forces reclassification and may dispatch on the same SHA when the resulting classification state is unchanged. Unrelated label events and repeated non-explicit unchanged classifications must not dispatch. Force mode bypasses policy gates but not classification prerequisites, evidence, validation, filesystem, citation, publication, or stale-head safeguards. +Lifecycle events include ready and draft transitions, closure, relevant labels, and requested, in-progress, and completed CI states. +Automation-written label events may reconcile lifecycle but never start automatic classification on their own. +Classification admission, review admission, result publication, and reconciliation use one per-pull-request mutation boundary. +The workflow and mutation boundary retain queued wake-ups with non-canceling `queue: max` concurrency, so stale work revalidates and no-ops without interrupting active publication. +An admitted classifier that reaches draft publishes nothing. +An admitted review may publish its validated exact-head review, successful outcome check, and single count label while draft or closed-unmerged when all authority, CI, policy, and lease guards still pass. +Closed and merged pull requests never admit new automatic work, and merged pull requests publish no in-flight result. +If review creation is rejected after closed-unmerged validation, complete the lease neutral without a partial publication. + ## Run summary Link the resolved pull request from the workflow run summary, on every route. diff --git a/.github/workflows/pr-automation.yml b/.github/workflows/pr-automation.yml index 2596d893a..0e56cec93 100644 --- a/.github/workflows/pr-automation.yml +++ b/.github/workflows/pr-automation.yml @@ -16,10 +16,10 @@ run-name: >- on: pull_request_target: - types: [opened, reopened, synchronize, ready_for_review, edited, labeled, unlabeled] + types: [opened, reopened, synchronize, ready_for_review, converted_to_draft, closed, edited, labeled, unlabeled] workflow_run: workflows: [CI] - types: [completed] + types: [requested, in_progress, completed] repository_dispatch: types: [pr-automation-classified] workflow_dispatch: @@ -40,16 +40,16 @@ on: type: boolean concurrency: - # Keep event routes separate so a stale CI completion cannot cancel newer PR-head automation. - # Group CI completions by source commit because their associated PR may already have moved ahead. - # Non-request labels get their own group, so their no-op runs cannot cancel active automation. + # Keep event routes separate while retaining every bounded pending wake-up. + # Admission revalidates current state, so stale queued work safely becomes a no-op. group: >- pr-automation-${{ github.event_name }}-${{ github.event.workflow_run.head_sha || github.event.pull_request.number || inputs.pr-number || github.event.client_payload.pr_number || - github.run_id }}-${{ github.event.label.name == 'ai-review/allow-oversized' && 'pr' || - github.event.label.name && format('label-{0}', github.event.label.name) || 'pr' }} - cancel-in-progress: true + github.run_id }}-${{ github.event.label.name && + format('label-{0}', github.event.label.name) || 'pr' }} + cancel-in-progress: false + queue: max permissions: {} @@ -75,7 +75,6 @@ jobs: review-route: ${{ steps.resolve.outputs.review-route }} classifier-lane: ${{ steps.resolve.outputs.classifier-lane }} reviewer-pipeline-lane: ${{ steps.resolve.outputs.reviewer-pipeline-lane }} - evidence-max-bytes: ${{ steps.resolve.outputs.evidence-max-bytes }} steps: - uses: actions/checkout@v6 with: @@ -112,7 +111,6 @@ jobs: core.setOutput("review-route", result.reviewRoute === true); core.setOutput("classifier-lane", result.prNumber ? (Number(result.prNumber) % 4) + 1 : ""); core.setOutput("reviewer-pipeline-lane", result.prNumber ? (Number(result.prNumber) % 7) + 1 : ""); - core.setOutput("evidence-max-bytes", result.evidenceMaxBytes ?? ""); core.info(JSON.stringify({ event: "pr-automation.resolve-pr", decision: result })); if (result.prNumber) { const prUrl = `${process.env.PULL_REQUEST_URL_BASE}/${result.prNumber}`; @@ -148,7 +146,6 @@ jobs: env: HEAD_SHA: ${{ needs.resolve-pr.outputs.head-sha }} FORCE: ${{ needs.resolve-pr.outputs.force }} - RETRY_WITH_LARGER_EVIDENCE: ${{ needs.resolve-pr.outputs.review-requested }} with: script: | const { resolveClassificationGate } = require("./.github/pr-automation/classification-gate"); @@ -158,7 +155,6 @@ jobs: repo: context.repo.repo, expectedSha: process.env.HEAD_SHA, force: process.env.FORCE === "true", - retryWithLargerEvidence: process.env.RETRY_WITH_LARGER_EVIDENCE === "true", }); core.setOutput("required", decision.required); core.setOutput("available", decision.available); @@ -229,12 +225,14 @@ jobs: needs.classification-gate.outputs.required == 'true')) runs-on: ubuntu-latest concurrency: - group: pr-automation-admission-classification-${{ needs.resolve-pr.outputs.pr-number }} + group: pr-automation-mutation-${{ needs.resolve-pr.outputs.pr-number }} cancel-in-progress: false + queue: max permissions: actions: read checks: write contents: read + pull-requests: read outputs: owner: ${{ steps.claim.outputs.owner }} lease: ${{ steps.claim.outputs.lease }} @@ -248,6 +246,7 @@ jobs: uses: actions/github-script@v8 env: HEAD_SHA: ${{ needs.resolve-pr.outputs.head-sha }} + PULL_REQUEST_NUMBER: ${{ needs.resolve-pr.outputs.pr-number }} FORCE: ${{ needs.resolve-pr.outputs.force }} ALLOW_SUCCESS: ${{ needs.resolve-pr.outputs.review-requested }} RUN_ID: ${{ github.run_id }} @@ -263,6 +262,7 @@ jobs: const decision = await claimAutomaticLease({ github, owner: context.repo.owner, repo: context.repo.repo, kind: "classification", headSha: process.env.HEAD_SHA, + prNumber: Number(process.env.PULL_REQUEST_NUMBER), runId: Number(process.env.RUN_ID), attempt: Number(process.env.RUN_ATTEMPT), allowSuccess: process.env.ALLOW_SUCCESS === "true", }); @@ -453,7 +453,6 @@ jobs: env: HEAD_SHA: ${{ needs.resolve-pr.outputs.head-sha }} BASE_SHA: ${{ needs.resolve-pr.outputs.base-sha }} - EVIDENCE_MAX_BYTES: ${{ needs.resolve-pr.outputs.evidence-max-bytes }} run: | set -euo pipefail export GIT_CONFIG_NOSYSTEM=1 @@ -464,7 +463,7 @@ jobs: git fetch --no-tags origin +master:refs/remotes/origin/master git checkout --detach origin/master if ! bash ./.github/pr-automation/fetch-pr-evidence.sh \ - "$HEAD_SHA" "$BASE_SHA" "$EVIDENCE_MAX_BYTES"; then + "$HEAD_SHA" "$BASE_SHA"; then reason="pull request evidence unavailable" if test -f pr-evidence/failure-reason.txt; then reason="$(cat pr-evidence/failure-reason.txt)" @@ -587,7 +586,7 @@ jobs: event: "pr-automation.classification-state", decision: { ok: state.ok, mode: state.mode, failed: state.failed === true, - oversized: state.oversized === true, expectedSha: state.expectedSha, + expectedSha: state.expectedSha, }, })); @@ -673,7 +672,7 @@ jobs: const classificationCheck = classificationValid && classification.output?.title === "Classification complete" && protocolState.automaticReviewEligible === true; - const classificationId = classificationCheck ? classification.id : null; + const classificationId = classificationValid ? classification.id : null; const legitimacyStopped = classificationOwned && classification.output?.title === "Automation stopped"; const protocolRelated = protocolState?.protocolRelated === true; @@ -764,12 +763,15 @@ jobs: needs.review-gate.outputs.eligible == 'true' runs-on: ubuntu-latest concurrency: - group: pr-automation-admission-review-${{ needs.resolve-pr.outputs.pr-number }} + group: pr-automation-mutation-${{ needs.resolve-pr.outputs.pr-number }} cancel-in-progress: false + queue: max permissions: actions: read checks: write contents: read + issues: read + pull-requests: read outputs: owner: ${{ steps.claim.outputs.owner }} lease: ${{ steps.claim.outputs.lease }} @@ -783,6 +785,8 @@ jobs: uses: actions/github-script@v8 env: HEAD_SHA: ${{ needs.resolve-pr.outputs.head-sha }} + PULL_REQUEST_NUMBER: ${{ needs.resolve-pr.outputs.pr-number }} + GATE: ${{ needs.review-gate.outputs.gate }} FORCE: ${{ needs.resolve-pr.outputs.force }} RUN_ID: ${{ github.run_id }} RUN_ATTEMPT: ${{ github.run_attempt }} @@ -794,9 +798,11 @@ jobs: return; } const { claimAutomaticLease } = require("./.github/pr-automation/automation-lease"); + const gate = JSON.parse(process.env.GATE || "{}"); const decision = await claimAutomaticLease({ github, owner: context.repo.owner, repo: context.repo.repo, kind: "review", headSha: process.env.HEAD_SHA, + prNumber: Number(process.env.PULL_REQUEST_NUMBER), gate, runId: Number(process.env.RUN_ID), attempt: Number(process.env.RUN_ATTEMPT), }); core.setOutput("owner", decision.owner); @@ -835,7 +841,6 @@ jobs: lease: ${{ needs.review-admission.outputs.lease }} specialist-reviewers: ${{ needs.review-gate.outputs.specialist-reviewers }} required-reviewers: ${{ needs.review-gate.outputs.required-reviewers }} - evidence-max-bytes: ${{ needs.resolve-pr.outputs.evidence-max-bytes }} # Work around actions/runner#4453: environment secrets otherwise resolve empty in reusable jobs. # This exposes caller-accessible secrets, so the called workflow must remain trusted and not execute PR code. secrets: inherit @@ -1022,11 +1027,12 @@ jobs: name: Write pull request state needs: [resolve-pr, classification-gate, classification-admission, deterministic-analysis, fork-rate-limit, semver, classifier, resolve-classification-state, review-gate, review-admission, review-pipeline, resolve-review-state] - if: always() && !cancelled() && needs.resolve-pr.outputs.ok == 'true' + if: always() && !cancelled() && needs.resolve-pr.outputs.pr-number != '' runs-on: ubuntu-latest concurrency: - group: pr-automation-write-${{ needs.resolve-pr.outputs.pr-number }} + group: pr-automation-mutation-${{ needs.resolve-pr.outputs.pr-number }} cancel-in-progress: false + queue: max permissions: actions: read contents: write @@ -1051,6 +1057,7 @@ jobs: env: PULL_REQUEST_NUMBER: ${{ needs.resolve-pr.outputs.pr-number }} REVIEW_REQUESTED: ${{ needs.resolve-pr.outputs.review-requested }} + OBSERVED_CI_RUN: ${{ needs.resolve-pr.outputs.observed-ci-run }} CLASSIFICATION_STATE: ${{ needs.resolve-classification-state.outputs.state }} UPSTREAM_RESULTS: >- {"classification-gate":"${{ needs.classification-gate.result }}","deterministic-analysis":"${{ needs.deterministic-analysis.result }}","fork-rate-limit":"${{ needs.fork-rate-limit.result }}","semver":"${{ needs.semver.result }}","classifier":"${{ needs.classifier.result }}","resolve-classification-state":"${{ needs.resolve-classification-state.result }}","review-gate":"${{ needs.review-gate.result }}","review-pipeline":"${{ needs.review-pipeline.result }}","resolve-review-state":"${{ needs.resolve-review-state.result }}"} @@ -1058,6 +1065,8 @@ jobs: script: | const fs = require("node:fs"); const { writeState } = require("./.github/pr-automation/write-state"); + const { StaleHeadError } = require("./.github/pr-automation/current-head"); + const { reconcileLifecycle } = require("./.github/pr-automation/lifecycle"); const parse = (value) => { try { return JSON.parse(value || ""); } catch { return null; } }; const readState = (file) => { try { return parse(fs.readFileSync(file, "utf8")); } catch { return null; } @@ -1073,17 +1082,29 @@ jobs: expectedSha: state.expectedSha, } : null, })); - if (!state) { - core.warning("No normalized state was available; no PR mutation was attempted"); - return; + try { + if (!state) { + core.info("No normalized result state was available; reconciling lifecycle only"); + } else { + await writeState({ + github, owner: context.repo.owner, repo: context.repo.repo, + prNumber: Number(process.env.PULL_REQUEST_NUMBER), state, + botLogin: "github-actions[bot]", + reviewRequested: process.env.REVIEW_REQUESTED === "true", + }); + } + } catch (error) { + if (!(error instanceof StaleHeadError)) throw error; + core.info("Result publication became stale; reconciling lifecycle only"); + } finally { + const lifecycle = await reconcileLifecycle({ + github, owner: context.repo.owner, repo: context.repo.repo, + prNumber: Number(process.env.PULL_REQUEST_NUMBER), + observedRun: parse(process.env.OBSERVED_CI_RUN), + }); + core.info(JSON.stringify({ event: "pr-automation.lifecycle", lifecycle })); } - await writeState({ - github, owner: context.repo.owner, repo: context.repo.repo, - prNumber: Number(process.env.PULL_REQUEST_NUMBER), state, - botLogin: "github-actions[bot]", - reviewRequested: process.env.REVIEW_REQUESTED === "true", - }); core.info(JSON.stringify({ event: "pr-automation.write-state-complete", - decision: { expectedSha: state.expectedSha, mode: state.mode }, + decision: state ? { expectedSha: state.expectedSha, mode: state.mode } : null, })); diff --git a/.github/workflows/review-pipeline.yml b/.github/workflows/review-pipeline.yml index 90808ca5b..bc8a24903 100644 --- a/.github/workflows/review-pipeline.yml +++ b/.github/workflows/review-pipeline.yml @@ -19,10 +19,6 @@ on: description: Canonically ordered specialist reviewer IDs required: true type: string - evidence-max-bytes: - description: Trusted maximum evidence diff size - required: true - type: string required-reviewers: description: Specialist reviewer IDs the caller requires to succeed required: false @@ -67,7 +63,6 @@ jobs: env: HEAD_SHA: ${{ inputs.head-sha }} BASE_SHA: ${{ inputs.base-sha }} - EVIDENCE_MAX_BYTES: ${{ inputs.evidence-max-bytes }} WORKFLOW_SHA: ${{ github.workflow_sha }} run: | set -euo pipefail @@ -79,7 +74,7 @@ jobs: git fetch --no-tags origin "+$WORKFLOW_SHA:refs/remotes/origin/automation" git checkout --detach origin/automation if ! bash ./.github/pr-automation/fetch-pr-evidence.sh \ - "$HEAD_SHA" "$BASE_SHA" "$EVIDENCE_MAX_BYTES"; then + "$HEAD_SHA" "$BASE_SHA"; then reason="pull request evidence unavailable" if test -f pr-evidence/failure-reason.txt; then reason="$(cat pr-evidence/failure-reason.txt)"