From 063e2bb1b92423463ac1dc127532ea896d653703 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 2 Oct 2026 01:17:37 +0900 Subject: [PATCH 1/5] ci(pr-automation): reconcile lifecycle state Derive next-actor labels from current PR, CI, lease, and trusted exact-head review state instead of individual event transitions. Preserve already-admitted valid reviews across draft and closed-unmerged states while stale, merged, ambiguous, and partial states fail closed. --- .github/PR_AUTOMATION.md | 46 +- .github/pr-automation/automation-lease.js | 97 ++- .github/pr-automation/automation.test.js | 963 ++++++++++++++++++---- .github/pr-automation/ci-state.js | 25 +- .github/pr-automation/current-head.js | 22 +- .github/pr-automation/lifecycle.js | 131 +++ .github/pr-automation/resolve-pr.js | 68 +- .github/pr-automation/resolve-state.js | 74 +- .github/pr-automation/review-outcome.js | 40 + .github/pr-automation/write-state.js | 255 +++++- .github/workflows/pr-automation.intent.md | 26 +- .github/workflows/pr-automation.yml | 69 +- 12 files changed, 1497 insertions(+), 319 deletions(-) create mode 100644 .github/pr-automation/lifecycle.js create mode 100644 .github/pr-automation/review-outcome.js diff --git a/.github/PR_AUTOMATION.md b/.github/PR_AUTOMATION.md index 623ac39305..8be9e4aff2 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 @@ -257,9 +263,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 +284,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 914f0a0cc7..9933052a8d 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 { 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 +4135,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 +4241,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 +4257,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 +4318,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 +4355,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 +4383,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 +4466,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 +4474,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 +4497,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 +4517,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 +4549,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"], @@ -5180,14 +5781,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 +5816,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 +5841,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 +6040,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 +6051,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 +6207,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 +6239,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 +6248,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 +6284,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 +6419,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 +6472,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 fb5576b630..a8b34c44d0 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/current-head.js b/.github/pr-automation/current-head.js index 60158236c4..db519d7c80 100644 --- a/.github/pr-automation/current-head.js +++ b/.github/pr-automation/current-head.js @@ -21,11 +21,27 @@ async function readPullRequest({ github, owner, repo, pullNumber }) { } } -async function assertCurrentHead({ github, owner, repo, pullNumber, expectedHeadSha }) { +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) { + const closedUnmerged = data.state === "closed" && !data.merged && !data.merged_at; + if ((data.state !== "open" && !(allowClosedUnmerged && closedUnmerged)) || + data.head?.sha !== expectedHeadSha) { throw new StaleHeadError(); } + return data; } -module.exports = { StaleHeadError, assertCurrentHead }; +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, isOpenNonDraftAtHead, isOpenNonDraftAtHeadNow, +}; diff --git a/.github/pr-automation/lifecycle.js b/.github/pr-automation/lifecycle.js new file mode 100644 index 0000000000..4ee4e86008 --- /dev/null +++ b/.github/pr-automation/lifecycle.js @@ -0,0 +1,131 @@ +"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 = ["needs-review", "needs-author-action"]; + +function names(labels) { + return new Set((labels || []).map((label) => typeof label === "string" ? label : label?.name).filter(Boolean)); +} + +function canonical(runs, kind, headSha) { + return canonicalRuns(runs || [], { kind, headSha }); +} + +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 = names((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 = canonical(classificationRuns, "classification", headSha); + const reviews = canonical(reviewRuns, "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 = { + ACTOR_LABELS, canonical, lifecycleActor, readLifecycleSnapshot, reconcileLifecycle, +}; diff --git a/.github/pr-automation/resolve-pr.js b/.github/pr-automation/resolve-pr.js index 88216de1ab..a76344b4f3 100644 --- a/.github/pr-automation/resolve-pr.js +++ b/.github/pr-automation/resolve-pr.js @@ -4,8 +4,17 @@ 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 +36,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 +47,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 +57,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; @@ -67,25 +76,29 @@ async function resolvePr({ github, context, inputs = {} }) { 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,24 +111,31 @@ 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 unrelatedLabel = route === "classification" && ["labeled", "unlabeled"].includes(context.payload.action) && + context.payload.label?.name !== OVERSIZED_REVIEW_LABEL; + if (unrelatedLabel) return noResult("unrelated pull request label", route, pr); const reviewRequested = oversizedReviewRequested || dispatchReview; const classificationRequested = route === "classification" || (route === "dispatch" && !dispatchReview); diff --git a/.github/pr-automation/resolve-state.js b/.github/pr-automation/resolve-state.js index a14cd4362f..f65b2b2d62 100644 --- a/.github/pr-automation/resolve-state.js +++ b/.github/pr-automation/resolve-state.js @@ -28,24 +28,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) { @@ -97,7 +91,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 +179,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 = [ @@ -258,8 +252,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 +278,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 +345,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 +356,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, }; diff --git a/.github/pr-automation/review-outcome.js b/.github/pr-automation/review-outcome.js new file mode 100644 index 0000000000..eef7405738 --- /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 62695a13cd..69b86997bd 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 { + hasCurrentValidClassification, ownsActiveLease, parseLeaseMarker, recheckAutomaticReview, +} = require("./automation-lease"); +const { + assertCurrentHead, isOpenNonDraftAtHead, 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 pull.state === "closed" && !pull.merged && !pull.merged_at && 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)); @@ -50,7 +64,7 @@ function markerBody(comment, owner, repo) { } 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 +72,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 +85,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 +176,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 +232,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 +265,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 +311,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 +342,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 +385,79 @@ 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 canPublishReview = async () => { + if (!await canMutate()) return false; + if (state.failed === true || state.blocked === true) return true; + if (state.forced === true) { + return await hasCurrentValidClassification({ + github, owner, repo, headSha: state.expectedSha, + classificationId: state.admittedGate?.classificationId, + }); + } + // 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 hasCurrentValidClassification({ + github, owner, repo, headSha: state.expectedSha, classificationId: state.admittedGate?.classificationId, + }); + }; + 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 +491,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 +517,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 +595,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 02c10ec2ca..b38f5e977a 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. @@ -111,6 +112,15 @@ Unrelated label events and repeated non-explicit unchanged classifications must 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 2596d893a1..e44e4e6ed2 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 + cancel-in-progress: false + queue: max permissions: {} @@ -229,12 +229,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 +250,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 +266,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", }); @@ -673,7 +677,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 +768,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 +790,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 +803,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); @@ -1022,11 +1033,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 +1063,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 +1071,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 +1088,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, })); From c398aaa9b742bfc123d7867a479800aeeecf05e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 2 Oct 2026 01:47:16 +0900 Subject: [PATCH 2/5] ci(pr-automation): simplify lifecycle routing Remove the label-based 4 MiB evidence override and keep one fixed 1 MiB fail-closed evidence limit. Preserve per-label lifecycle scheduling and avoid a redundant CI-route pull request lookup. --- .github/PR_AUTOMATION.md | 5 +- .github/pr-automation/automation.test.js | 103 ++++--------------- .github/pr-automation/classification-gate.js | 4 +- .github/pr-automation/fetch-pr-evidence.sh | 16 +-- .github/pr-automation/labels.json | 5 - .github/pr-automation/resolve-pr.js | 10 +- .github/pr-automation/resolve-state.js | 13 +-- .github/pr-automation/write-state.js | 5 +- .github/workflows/pr-automation.intent.md | 6 +- .github/workflows/pr-automation.yml | 14 +-- .github/workflows/review-pipeline.yml | 7 +- 11 files changed, 41 insertions(+), 147 deletions(-) diff --git a/.github/PR_AUTOMATION.md b/.github/PR_AUTOMATION.md index 8be9e4aff2..067925aa8b 100644 --- a/.github/PR_AUTOMATION.md +++ b/.github/PR_AUTOMATION.md @@ -251,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 1 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. diff --git a/.github/pr-automation/automation.test.js b/.github/pr-automation/automation.test.js index e578d1dd20..138b5ada44 100644 --- a/.github/pr-automation/automation.test.js +++ b/.github/pr-automation/automation.test.js @@ -21,7 +21,7 @@ const { reviewCount, reviewOutcome, reviewPolicyEligible, OVERLAP_MARKER, OVERLAP_LABEL, EVIDENCE_LIMIT_MARKER, LEGACY_XL_MARKER, LEGITIMACY_LABEL, - LEGITIMACY_MARKER_PREFIX, OVERSIZED_MARKER, OVERSIZED_REVIEW_LABEL, contributorEligibility, + LEGITIMACY_MARKER_PREFIX, contributorEligibility, } = require("./resolve-state"); const { resolvePr } = require("./resolve-pr"); const { resolveClassificationGate } = require("./classification-gate"); @@ -1379,7 +1379,7 @@ test("review skip summary explains gate and quota failures", () => { }), ["The workflow's automated review conditions were not satisfied."]); }); -test("classification gate reuses paginated completed state but forces oversized retries", async () => { +test("classification gate reuses paginated completed state", async () => { let reads = 0; const machineState = { protocolRelated: false, risk: "low", specialistReviewers: [], @@ -1412,14 +1412,6 @@ test("classification gate reuses paginated completed state but forces oversized assert.equal(cached.required, false); assert.equal(reads, 1); - const retry = await resolveClassificationGate({ ...args, retryWithLargerEvidence: true }); - assert.deepEqual(retry, { - available: true, required: true, reason: "", - externalId: `${CLASSIFIER_SCHEMA_VERSION}:${SHA}`, - completed: true, - }); - assert.equal(reads, 2); - const unavailable = await resolveClassificationGate({ ...args, github: { @@ -1576,12 +1568,10 @@ test("LLM evidence is bound to the resolved pull request base", () => { const evidenceScript = fs.readFileSync(path.join(__dirname, "fetch-pr-evidence.sh"), "utf8"); const classifier = workflowJob(workflow, "classifier"); assert.match(classifier, /BASE_SHA: \$\{\{ needs\.resolve-pr\.outputs\.base-sha \}\}/); - assert.match(classifier, - /fetch-pr-evidence\.sh \\\n\s+"\$HEAD_SHA" "\$BASE_SHA" "\$EVIDENCE_MAX_BYTES"/); + assert.match(classifier, /fetch-pr-evidence\.sh \\\n\s+"\$HEAD_SHA" "\$BASE_SHA"/); const evidence = workflowJob(reviewWorkflow, "evidence"); assert.match(evidence, /BASE_SHA: \$\{\{ inputs\.base-sha \}\}/); - assert.match(evidence, - /fetch-pr-evidence\.sh \\\n\s+"\$HEAD_SHA" "\$BASE_SHA" "\$EVIDENCE_MAX_BYTES"/); + assert.match(evidence, /fetch-pr-evidence\.sh \\\n\s+"\$HEAD_SHA" "\$BASE_SHA"/); assert.match(evidenceScript, /\+\$base_sha:refs\/remotes\/origin\/pull-request-base/); assert.match( evidenceScript, @@ -1603,20 +1593,17 @@ test("evidence caps are trusted, bounded, and fail closed with guidance", () => assert.match(evidenceAttributes, /^\* !diff$/m); assert.match(evidenceAttributes, /^\.github\/actions\/openai-agent\/dist\/\*\* -diff$/m); assert.match(evidenceScript, /failure-reason\.txt/); - assert.match(evidenceScript, /1048576\) limit_mib=1/); - assert.match(evidenceScript, /4194304\) limit_mib=4/); - assert.match(evidenceScript, /invalid evidence diff limit/); + assert.match(evidenceScript, /max_bytes=1048576/); + assert.match(evidenceScript, /max_bytes=1048576/); + assert.doesNotMatch(evidenceScript, /4194304/); assert.match(evidenceScript, /-gt "\$max_bytes"/); assert.match(evidenceScript, /exit 1/); assert.doesNotMatch(evidenceScript, /pull-request\.diff\.truncated/); const classifier = workflowJob(workflow, "classifier"); assert.match(classifier, /id: evidence/); - assert.match(classifier, - /EVIDENCE_MAX_BYTES: \$\{\{ needs\.resolve-pr\.outputs\.evidence-max-bytes \}\}/); assert.match(classifier, /steps\.evidence\.outputs\.failure-reason \|\|/); const evidence = workflowJob(reviewWorkflow, "evidence"); assert.match(evidence, /id: evidence/); - assert.match(evidence, /EVIDENCE_MAX_BYTES: \$\{\{ inputs\.evidence-max-bytes \}\}/); assert.match(evidence, /failure-reason: \$\{\{ steps\.record\.outputs\.failure-reason \}\}/); assert.match(evidence, /EVIDENCE_REASON: \$\{\{ steps\.evidence\.outputs\.failure-reason \}\}/); assert.match(workflowJob(reviewWorkflow, "validate"), @@ -1627,30 +1614,15 @@ test("evidence caps are trusted, bounded, and fail closed with guidance", () => sizeLabels: ["size/XL", "size/XXL"], firstTime: false, }; const classification = resolveClassificationState({ - expectedSha: SHA, labels: [OVERSIZED_REVIEW_LABEL], deterministic, + expectedSha: SHA, labels: [], deterministic, classifierReason: reason, semver: { head_sha: SHA, status: "not-suspected" }, }); assert.equal(classification.failed, true); assert.deepEqual(classification.comments, [{ - kind: "evidence-limit", marker: EVIDENCE_LIMIT_MARKER, limitMiB: 1, + kind: "evidence-limit", marker: EVIDENCE_LIMIT_MARKER, }]); assert.match(markerBody(classification.comments[0]), /No model was invoked with partial evidence/); - assert.match(markerBody(classification.comments[0]), /ai-review\/allow-oversized/); - - const reviewFailure = resolveReviewState({ - expectedSha: SHA, labels: [], gate: { - ok: true, force: true, head_sha: SHA, classificationValid: true, - classificationId: FORCED_CLASSIFICATION_ID, - protocolRelated: false, risk: "unknown", specialistReviewers: ["skeptical"], - }, - reviewerReason: "pull request diff exceeds the 4 MiB evidence limit", - force: true, reviewMarkerId: "1", - }); - assert.equal(reviewFailure.failed, true); - assert.deepEqual(reviewFailure.comments, [{ - kind: "evidence-limit", marker: EVIDENCE_LIMIT_MARKER, limitMiB: 4, - }]); - assert.match(markerBody(reviewFailure.comments[0]), /runtime maximum/); + assert.match(markerBody(classification.comments[0]), /split the change/); }); test("every deterministic label is declared and the repository rules classify tooling changes", () => { @@ -2504,7 +2476,6 @@ test("bot authors are excluded from automation", async () => { const human = await resolve({ node_id: "U_3", login: "contributor", type: "User" }); assert.equal(human.ok, true); assert.equal(human.reviewRoute, true); - assert.equal(human.evidenceMaxBytes, 1024 * 1024); const labels = new Set(["needs-review"]); const listChecks = () => {}; @@ -2699,16 +2670,16 @@ test("pull request events request classification without lifecycle metadata", as } }); -test("only oversized-review label changes start automation from label events", async () => { +test("label changes do not start automation", async () => { const pullRequest = (labels = []) => ({ number: 7, draft: false, state: "open", labels, user: { node_id: "U_1", login: "contributor", type: "User" }, head: { sha: SHA, repo: { full_name: "Devolutions/IronRDP" } }, base: { sha: "b".repeat(40) }, }); - const resolve = async (label, action = "labeled", labels = [OVERSIZED_REVIEW_LABEL]) => resolvePr({ + const resolve = async (label, action = "labeled") => resolvePr({ github: { rest: { pulls: { - get: async () => ({ data: pullRequest(labels) }), - list: async () => ({ data: [pullRequest(labels)] }), + get: async () => ({ data: pullRequest() }), + list: async () => ({ data: [pullRequest()] }), } } }, context: { eventName: "pull_request_target", repo: { owner: "Devolutions", repo: "IronRDP" }, @@ -2716,22 +2687,10 @@ test("only oversized-review label changes start automation from label events", a }, }); - const requested = await resolve(OVERSIZED_REVIEW_LABEL); - assert.equal(requested.ok, true); - assert.equal(requested.classificationRequested, true); - assert.equal(requested.reviewRequested, true); - assert.equal(requested.force, false); - assert.equal(requested.evidenceMaxBytes, 4 * 1024 * 1024); - const revoked = await resolve(OVERSIZED_REVIEW_LABEL, "unlabeled", []); - assert.equal(revoked.ok, true); - assert.equal(revoked.classificationRequested, true); - assert.equal(revoked.reviewRequested, false); - assert.equal(revoked.evidenceMaxBytes, 1024 * 1024); - assert.equal((await resolve("breaking-change")).reason, "unrelated pull request label"); - assert.equal((await resolve("needs-review")).reason, "unrelated pull request label"); - assert.equal((await resolve(FAILURE_LABEL)).reason, "unrelated pull request label"); - assert.equal((await resolve("size/XXL")).reason, "unrelated pull request label"); - assert.equal((await resolve("size/XXL", "unlabeled", [])).reason, "unrelated pull request label"); + for (const label of ["breaking-change", "needs-review", FAILURE_LABEL, "size/XXL"]) { + assert.equal((await resolve(label)).reason, "unrelated pull request label"); + assert.equal((await resolve(label, "unlabeled")).reason, "unrelated pull request label"); + } }); test("deterministic semver outranks the model and a model-only break cannot stay low", () => { @@ -2945,26 +2904,6 @@ test("review publication applies the same policy the workflow spent its call on" assert.equal(resolveReviewState({ ...args, labels: ["risk/low", "size/XXL"], gate: gate({ protocolRelated: true, risk: "low" }), }).failed, undefined); - assert.equal(resolveReviewState({ - ...args, labels: ["risk/low", "size/XXL", OVERSIZED_REVIEW_LABEL], - gate: gate({ protocolRelated: true, risk: "low" }), - }).failed, undefined); -}); - -test("persistent oversized-review label does not alter normal classification", () => { - const deterministic = { ok: true, pathLabels: [], ownedPathLabels: [], - sizeLabel: "size/XXL", sizeLabels: ["size/XL", "size/XXL"], firstTime: false }; - const state = resolveClassificationState({ - expectedSha: SHA, labels: [OVERSIZED_REVIEW_LABEL], deterministic, classifier: classifier({ - protocol_related: true, - }), semver: { head_sha: SHA, status: "not-suspected" }, - }); - - assert.equal(state.oversized, undefined); - assert.equal(state.check.title, "Classification complete"); - assert.equal(state.dispatchReview, true); - assert.deepEqual(state.comments, []); - assert.equal(state.removeCommentMarkers.includes(OVERSIZED_MARKER), true); }); test("size/XXL remains informational and does not suppress classification", () => { @@ -2986,7 +2925,6 @@ test("size/XXL remains informational and does not suppress classification", () = assert.equal(state.check.machineState.automaticReviewEligible, true); assert.equal(parseCheckState(`${state.check.summary}\n\n${encodeCheckState(state.check.machineState)}`) .automaticReviewEligible, true); - assert.equal(state.removeCommentMarkers.includes(OVERSIZED_MARKER), true); }); test("suspected overlap is advisory and is withdrawn once it no longer holds", () => { @@ -3121,14 +3059,11 @@ test("forced classification bypasses policy, quota, and cache but still validate const state = resolveClassificationState(args); assert.equal(state.failed, undefined); assert.equal(state.forced, true); - assert.equal(state.oversized, undefined); assert.equal(state.check.title, "Classification complete"); assert.equal(state.dispatchReview, false); assert.equal(state.check.machineState.automaticReviewEligible, false); assert.deepEqual(desiredLabels(state, "needs-author-action"), []); assert.deepEqual(desiredLabels(state, FAILURE_LABEL), [FAILURE_LABEL]); - assert.equal(state.comments.some((comment) => comment.kind === "oversized"), false); - assert.equal(state.removeCommentMarkers.includes(OVERSIZED_MARKER), true); const invalid = resolveClassificationState({ ...args, classifier: "" }); assert.equal(invalid.failed, true); @@ -5766,7 +5701,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"]) { diff --git a/.github/pr-automation/classification-gate.js b/.github/pr-automation/classification-gate.js index 574c214fb9..e1d78a9248 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/fetch-pr-evidence.sh b/.github/pr-automation/fetch-pr-evidence.sh index 598f3e4137..0bc2964186 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=1048576 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 1 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 3567fd9f25..c767a0c19e 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/resolve-pr.js b/.github/pr-automation/resolve-pr.js index a76344b4f3..d33e4f5124 100644 --- a/.github/pr-automation/resolve-pr.js +++ b/.github/pr-automation/resolve-pr.js @@ -1,7 +1,5 @@ "use strict"; -const { OVERSIZED_REVIEW_LABEL } = require("./resolve-state"); - const SHA = /^[0-9a-f]{40}$/; function noResult(reason, route = "unknown", pr = null, observedCiRun = null) { @@ -70,8 +68,6 @@ 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 { @@ -133,16 +129,14 @@ async function resolvePr({ github, context, inputs = {} }) { const labels = (pr.labels || []) .map((label) => typeof label === "string" ? label : label.name) .filter(Boolean); - const unrelatedLabel = route === "classification" && ["labeled", "unlabeled"].includes(context.payload.action) && - context.payload.label?.name !== OVERSIZED_REVIEW_LABEL; + const unrelatedLabel = route === "classification" && ["labeled", "unlabeled"].includes(context.payload.action); if (unrelatedLabel) return noResult("unrelated pull request label", route, pr); - const reviewRequested = oversizedReviewRequested || dispatchReview; + 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 f65b2b2d62..41b11b695f 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 1 MiB evidence limit$/; function labelsOf(labels) { return new Set((labels || []).map((label) => typeof label === "string" ? label : label?.name).filter(Boolean)); @@ -53,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; } @@ -206,7 +203,6 @@ function resolveClassificationState({ EVIDENCE_LIMIT_MARKER, FORK_QUOTA_MARKER, GLOBAL_QUOTA_MARKER, - OVERSIZED_MARKER, LEGACY_XL_MARKER, ], check: { @@ -396,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, contributorEligibility, resolveClassificationState, resolveReviewState, reviewCount, reviewOutcome, reviewPolicyEligible, }; diff --git a/.github/pr-automation/write-state.js b/.github/pr-automation/write-state.js index 69b86997bd..a22554c154 100644 --- a/.github/pr-automation/write-state.js +++ b/.github/pr-automation/write-state.js @@ -55,10 +55,7 @@ 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 1 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"); } diff --git a/.github/workflows/pr-automation.intent.md b/.github/workflows/pr-automation.intent.md index b38f5e977a..5fb9e3b405 100644 --- a/.github/workflows/pr-automation.intent.md +++ b/.github/workflows/pr-automation.intent.md @@ -102,12 +102,10 @@ 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 1 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. diff --git a/.github/workflows/pr-automation.yml b/.github/workflows/pr-automation.yml index e44e4e6ed2..0e56cec935 100644 --- a/.github/workflows/pr-automation.yml +++ b/.github/workflows/pr-automation.yml @@ -46,8 +46,8 @@ concurrency: 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' }} + github.run_id }}-${{ github.event.label.name && + format('label-{0}', github.event.label.name) || 'pr' }} cancel-in-progress: false queue: max @@ -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); @@ -457,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 @@ -468,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)" @@ -591,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, }, })); @@ -846,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 diff --git a/.github/workflows/review-pipeline.yml b/.github/workflows/review-pipeline.yml index 90808ca5b9..bc8a249035 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)" From c4ecb8951e801c6005c0b36d89825f8fa3679d4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Thu, 8 Oct 2026 01:25:31 +0900 Subject: [PATCH 3/5] ci(pr-automation): use one 4 MiB evidence limit Keep the former opt-in maximum as the fixed classification and review diff cap without restoring the deleted authorization label. --- .github/PR_AUTOMATION.md | 2 +- .github/pr-automation/automation.test.js | 7 +++---- .github/pr-automation/fetch-pr-evidence.sh | 4 ++-- .github/pr-automation/resolve-state.js | 2 +- .github/pr-automation/write-state.js | 2 +- .github/workflows/pr-automation.intent.md | 2 +- 6 files changed, 9 insertions(+), 10 deletions(-) diff --git a/.github/PR_AUTOMATION.md b/.github/PR_AUTOMATION.md index 067925aa8b..6cb3170693 100644 --- a/.github/PR_AUTOMATION.md +++ b/.github/PR_AUTOMATION.md @@ -251,7 +251,7 @@ 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. +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. diff --git a/.github/pr-automation/automation.test.js b/.github/pr-automation/automation.test.js index 138b5ada44..e1f873b100 100644 --- a/.github/pr-automation/automation.test.js +++ b/.github/pr-automation/automation.test.js @@ -1586,16 +1586,15 @@ test("evidence caps are trusted, bounded, and fail closed with guidance", () => const reviewWorkflow = readReviewWorkflow(githubDirectory); const evidenceScript = fs.readFileSync(path.join(__dirname, "fetch-pr-evidence.sh"), "utf8"); const evidenceAttributes = fs.readFileSync(path.join(__dirname, "evidence-diff-attributes"), "utf8"); - const reason = "pull request diff exceeds the 1 MiB evidence limit"; + const reason = "pull request diff exceeds the 4 MiB evidence limit"; assert.match(evidenceScript, /pr-head\/\.git\/info\/attributes/); assert.match(evidenceScript, /evidence-diff-attributes/); assert.doesNotMatch(evidenceScript, /dist\/index\.js/); assert.match(evidenceAttributes, /^\* !diff$/m); assert.match(evidenceAttributes, /^\.github\/actions\/openai-agent\/dist\/\*\* -diff$/m); assert.match(evidenceScript, /failure-reason\.txt/); - assert.match(evidenceScript, /max_bytes=1048576/); - assert.match(evidenceScript, /max_bytes=1048576/); - assert.doesNotMatch(evidenceScript, /4194304/); + assert.match(evidenceScript, /max_bytes=4194304/); + assert.doesNotMatch(evidenceScript, /max_bytes=1048576/); assert.match(evidenceScript, /-gt "\$max_bytes"/); assert.match(evidenceScript, /exit 1/); assert.doesNotMatch(evidenceScript, /pull-request\.diff\.truncated/); diff --git a/.github/pr-automation/fetch-pr-evidence.sh b/.github/pr-automation/fetch-pr-evidence.sh index 0bc2964186..5a0e22eb07 100755 --- a/.github/pr-automation/fetch-pr-evidence.sh +++ b/.github/pr-automation/fetch-pr-evidence.sh @@ -15,7 +15,7 @@ fi head_sha="$1" base_sha="$2" -max_bytes=1048576 +max_bytes=4194304 script_dir="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" export GIT_CONFIG_NOSYSTEM=1 export GIT_CONFIG_GLOBAL=/dev/null @@ -51,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 1 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/resolve-state.js b/.github/pr-automation/resolve-state.js index 41b11b695f..7b4c57b29a 100644 --- a/.github/pr-automation/resolve-state.js +++ b/.github/pr-automation/resolve-state.js @@ -18,7 +18,7 @@ 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 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)); diff --git a/.github/pr-automation/write-state.js b/.github/pr-automation/write-state.js index a22554c154..6d3898677e 100644 --- a/.github/pr-automation/write-state.js +++ b/.github/pr-automation/write-state.js @@ -55,7 +55,7 @@ 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") { - return `${comment.marker}\n\nAutomated model analysis stopped because this pull request's diff exceeds the 1 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.`; + 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"); } diff --git a/.github/workflows/pr-automation.intent.md b/.github/workflows/pr-automation.intent.md index 5fb9e3b405..45379e55e3 100644 --- a/.github/workflows/pr-automation.intent.md +++ b/.github/workflows/pr-automation.intent.md @@ -102,7 +102,7 @@ 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. +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. From 5fb78cb3fccca1b4d2ea217f3d23e316e39f06d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Thu, 8 Oct 2026 02:33:36 +0900 Subject: [PATCH 4/5] ci(pr-automation): reuse lifecycle guards Share canonical classification, closed-unmerged, and label normalization rules while preserving the distinct receipt and cleanup lease checks. --- .github/pr-automation/automation-lease.js | 19 ++++-------- .github/pr-automation/automation.test.js | 36 ++++++++++++++++++++--- .github/pr-automation/current-head.js | 9 ++++-- .github/pr-automation/lifecycle.js | 17 ++++------- .github/pr-automation/resolve-state.js | 2 +- .github/pr-automation/write-state.js | 15 +++++----- 6 files changed, 57 insertions(+), 41 deletions(-) diff --git a/.github/pr-automation/automation-lease.js b/.github/pr-automation/automation-lease.js index 9933052a8d..31da41f8ed 100644 --- a/.github/pr-automation/automation-lease.js +++ b/.github/pr-automation/automation-lease.js @@ -5,7 +5,7 @@ const { readCheckRuns } = require("./check-runs"); const { matchesGeneration, readLatestExactHeadCiRun } = require("./ci-state"); const { reviewCount } = require("./resolve-state"); const { reviewPolicyEligible } = require("./routing"); -const { isOpenNonDraftAtHeadNow } = require("./current-head"); +const { isClosedUnmerged, isOpenNonDraftAtHeadNow } = require("./current-head"); const { parseCheckState } = require("./validate-classifier"); const APP = "github-actions"; @@ -125,21 +125,13 @@ async function ownsActiveLease({ github, owner, repo, lease }) { return await ownsLatestLease({ github, owner, repo, lease }) && await ownerIsActive(github, { ...lease, owner, repo }); } -async function hasCurrentClassification({ github, owner, repo, headSha, classificationId }) { - const classifications = await readCanonicalRuns({ - github, owner, repo, kind: "classification", headSha, - }); - const classification = classifications?.[0]; - return classification?.id === classificationId && classification?.conclusion === "success"; -} - -async function hasCurrentValidClassification({ github, owner, repo, headSha, classificationId }) { +async function hasCurrentClassification({ github, owner, repo, headSha, classificationId, requireValid = false }) { const classifications = await readCanonicalRuns({ github, owner, repo, kind: "classification", headSha, }); const classification = classifications?.[0]; return classification?.id === classificationId && classification?.conclusion === "success" && - parseCheckState(classification.output?.summary) !== null; + (!requireValid || parseCheckState(classification.output?.summary) !== null); } async function recheckAutomaticReview({ @@ -151,8 +143,7 @@ async function recheckAutomaticReview({ "receipt", "cleanup", ].includes(phase)) throw new Error("invalid review recheck phase"); const { data: pull } = await github.rest.pulls.get({ owner, repo, pull_number: prNumber }); - const closedUnmerged = pull.state === "closed" && !pull.merged && !pull.merged_at; - if ((pull.state !== "open" && !(allowClosedUnmerged && closedUnmerged)) || + if ((pull.state !== "open" && !(allowClosedUnmerged && isClosedUnmerged(pull))) || (!allowDraft && pull.draft) || pull.head?.sha !== headSha) return false; const reviews = await readCanonicalRuns({ github, owner, repo, kind: "review", headSha }); if (!reviews || !await hasCurrentClassification({ @@ -193,5 +184,5 @@ async function recheckAutomaticReview({ module.exports = { CHECKS, canonicalRuns, claimAutomaticLease, leaseMarker, ownerIsActive, ownsActiveLease, ownsLatestLease, - hasCurrentClassification, hasCurrentValidClassification, parseLeaseMarker, recheckAutomaticReview, + hasCurrentClassification, parseLeaseMarker, recheckAutomaticReview, }; diff --git a/.github/pr-automation/automation.test.js b/.github/pr-automation/automation.test.js index e1f873b100..338b337e68 100644 --- a/.github/pr-automation/automation.test.js +++ b/.github/pr-automation/automation.test.js @@ -27,7 +27,7 @@ const { resolvePr } = require("./resolve-pr"); const { resolveClassificationGate } = require("./classification-gate"); const { latestExactHeadCiRun, readLatestExactHeadCiRun } = require("./ci-state"); const { - claimAutomaticLease, hasCurrentValidClassification, leaseMarker, parseLeaseMarker, recheckAutomaticReview, + claimAutomaticLease, hasCurrentClassification, leaseMarker, parseLeaseMarker, recheckAutomaticReview, } = require("./automation-lease"); const { StalePolicyError, applyLabels, escapeMarkdown, markerBody, writeState, @@ -55,7 +55,7 @@ const { const { REPORT_VERSION, buildReport, parseReport, stageIds, stageOutcome, } = require("./review-report"); -const { StaleHeadError, assertCurrentHead } = require("./current-head"); +const { StaleHeadError, assertCurrentHead, isClosedUnmerged } = require("./current-head"); const { lifecycleActor, readLifecycleSnapshot, reconcileLifecycle } = require("./lifecycle"); const { encodeReviewOutcome, parseReviewOutcome, trustedReviewOutcome } = require("./review-outcome"); const { @@ -124,12 +124,40 @@ test("current classification guard fails closed when no canonical check exists", paginate: { iterator: async function* () { yield { data: [] }; } }, rest: { checks: { listForRef: listChecks } }, }; - assert.equal(await hasCurrentValidClassification({ + assert.equal(await hasCurrentClassification({ github, owner: "Devolutions", repo: "IronRDP", headSha: SHA, - classificationId: FORCED_CLASSIFICATION_ID, + classificationId: FORCED_CLASSIFICATION_ID, requireValid: true, }), false); }); +test("current classification validity requires one canonical read and a parseable receipt", async () => { + let reads = 0; + const check = validClassificationCheck(); + check.output.summary = "invalid receipt"; + const github = { + paginate: { iterator: async function* () { reads += 1; yield { data: [check] }; } }, + rest: { checks: { listForRef: () => {} } }, + }; + const input = { + github, owner: "Devolutions", repo: "IronRDP", headSha: SHA, + classificationId: FORCED_CLASSIFICATION_ID, + }; + assert.equal(await hasCurrentClassification(input), true); + assert.equal(reads, 1); + assert.equal(await hasCurrentClassification({ ...input, requireValid: true }), false); + assert.equal(reads, 2); + check.output.summary = validClassificationCheck().output.summary; + assert.equal(await hasCurrentClassification({ ...input, requireValid: true }), true); + assert.equal(reads, 3); +}); + +test("closed-unmerged eligibility excludes merged and open pull requests", () => { + assert.equal(isClosedUnmerged({ state: "closed", merged: false, merged_at: null }), true); + assert.equal(isClosedUnmerged({ state: "closed", merged: true, merged_at: null }), false); + assert.equal(isClosedUnmerged({ state: "closed", merged: false, merged_at: "2026-01-01" }), false); + assert.equal(isClosedUnmerged({ state: "open", merged: false, merged_at: null }), false); +}); + test("lifecycle reconciliation derives actor labels from the live state table", () => { const state = (changes = {}) => ({ state: "open", draft: false, activeLease: false, diff --git a/.github/pr-automation/current-head.js b/.github/pr-automation/current-head.js index db519d7c80..ad82145dd9 100644 --- a/.github/pr-automation/current-head.js +++ b/.github/pr-automation/current-head.js @@ -21,12 +21,15 @@ async function readPullRequest({ github, owner, repo, pullNumber }) { } } +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 }); - const closedUnmerged = data.state === "closed" && !data.merged && !data.merged_at; - if ((data.state !== "open" && !(allowClosedUnmerged && closedUnmerged)) || + if ((data.state !== "open" && !(allowClosedUnmerged && isClosedUnmerged(data))) || data.head?.sha !== expectedHeadSha) { throw new StaleHeadError(); } @@ -43,5 +46,5 @@ async function isOpenNonDraftAtHeadNow({ github, owner, repo, pullNumber, expect } module.exports = { - StaleHeadError, assertCurrentHead, isOpenNonDraftAtHead, isOpenNonDraftAtHeadNow, + StaleHeadError, assertCurrentHead, isClosedUnmerged, isOpenNonDraftAtHead, isOpenNonDraftAtHeadNow, }; diff --git a/.github/pr-automation/lifecycle.js b/.github/pr-automation/lifecycle.js index 4ee4e86008..2bad8a81f8 100644 --- a/.github/pr-automation/lifecycle.js +++ b/.github/pr-automation/lifecycle.js @@ -5,17 +5,10 @@ const { readLatestExactHeadCiRun } = require("./ci-state"); const { canonicalRuns, ownerIsActive, parseLeaseMarker } = require("./automation-lease"); const { parseCheckState } = require("./validate-classifier"); const { trustedReviewOutcome } = require("./review-outcome"); +const { labelsOf } = require("./resolve-state"); const ACTOR_LABELS = ["needs-review", "needs-author-action"]; -function names(labels) { - return new Set((labels || []).map((label) => typeof label === "string" ? label : label?.name).filter(Boolean)); -} - -function canonical(runs, kind, headSha) { - return canonicalRuns(runs || [], { kind, headSha }); -} - async function activeLease(github, owner, repo, run, headSha) { const lease = parseLeaseMarker(run?.output?.summary); return run?.status === "in_progress" && lease?.headSha === headSha && @@ -42,7 +35,7 @@ function lifecycleActor(snapshot) { 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 = names((await github.rest.issues.get({ owner, repo, issue_number: prNumber })).data.labels); + 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 }; @@ -52,8 +45,8 @@ async function readLifecycleSnapshot({ github, owner, repo, prNumber, observedRu readCheckRuns({ github, owner, repo, ref: headSha, checkName: "AI automated review" }), readLatestExactHeadCiRun({ github, owner, repo, expectedSha: headSha, observedRun, ...ciRetry }), ]); - const classifications = canonical(classificationRuns, "classification", headSha); - const reviews = canonical(reviewRuns, "review", headSha); + 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([ @@ -127,5 +120,5 @@ async function reconcileLifecycle({ github, owner, repo, prNumber, observedRun = } module.exports = { - ACTOR_LABELS, canonical, lifecycleActor, readLifecycleSnapshot, reconcileLifecycle, + ACTOR_LABELS, lifecycleActor, readLifecycleSnapshot, reconcileLifecycle, }; diff --git a/.github/pr-automation/resolve-state.js b/.github/pr-automation/resolve-state.js index 7b4c57b29a..013680820d 100644 --- a/.github/pr-automation/resolve-state.js +++ b/.github/pr-automation/resolve-state.js @@ -392,7 +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, RISK, + LEGITIMACY_MARKER_PREFIX, OVERLAP_LABEL, OVERLAP_MARKER, RISK, labelsOf, contributorEligibility, resolveClassificationState, resolveReviewState, reviewCount, reviewOutcome, reviewPolicyEligible, }; diff --git a/.github/pr-automation/write-state.js b/.github/pr-automation/write-state.js index 6d3898677e..ce1f811d03 100644 --- a/.github/pr-automation/write-state.js +++ b/.github/pr-automation/write-state.js @@ -9,10 +9,10 @@ const { } = require("./review-render"); const { reviewPolicyEligible } = require("./routing"); const { - hasCurrentValidClassification, ownsActiveLease, parseLeaseMarker, recheckAutomaticReview, + hasCurrentClassification, ownsActiveLease, parseLeaseMarker, recheckAutomaticReview, } = require("./automation-lease"); const { - assertCurrentHead, isOpenNonDraftAtHead, isOpenNonDraftAtHeadNow, + assertCurrentHead, isClosedUnmerged, isOpenNonDraftAtHead, isOpenNonDraftAtHeadNow, } = require("./current-head"); const { matchesGeneration, readLatestExactHeadCiRun } = require("./ci-state"); const { ACTOR_LABELS, AI_COUNTS, reviewCount } = require("./resolve-state"); @@ -28,7 +28,7 @@ class ClosedReviewRejectedError extends Error { async function isClosedUnmergedAtHead(github, owner, repo, prNumber, expectedSha) { const { data: pull } = await github.rest.pulls.get({ owner, repo, pull_number: prNumber }); - return pull.state === "closed" && !pull.merged && !pull.merged_at && pull.head?.sha === expectedSha; + return isClosedUnmerged(pull) && pull.head?.sha === expectedSha; } async function issueLabels(github, owner, repo, prNumber) { @@ -398,9 +398,9 @@ async function writeState({ if (!await canMutate()) return false; if (state.failed === true || state.blocked === true) return true; if (state.forced === true) { - return await hasCurrentValidClassification({ + return await hasCurrentClassification({ github, owner, repo, headSha: state.expectedSha, - classificationId: state.admittedGate?.classificationId, + classificationId: state.admittedGate?.classificationId, requireValid: true, }); } // A normal automatic lease must carry the admission facts it is continuing from. @@ -415,8 +415,9 @@ async function writeState({ const canPublishReviewPublication = async () => { if (state.forced !== true) return await canPublishReview(); if (!await canMutate()) return false; - return await hasCurrentValidClassification({ - github, owner, repo, headSha: state.expectedSha, classificationId: state.admittedGate?.classificationId, + return await hasCurrentClassification({ + github, owner, repo, headSha: state.expectedSha, + classificationId: state.admittedGate?.classificationId, requireValid: true, }); }; const canPublishReviewReceipt = async () => { From d6ee62a74228c39c9d86c945bc48dbbb5267133c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Thu, 8 Oct 2026 03:03:32 +0900 Subject: [PATCH 5/5] ci(pr-automation): reuse terminal review guards Drop unused exports and duplicate actor-label and forced-classification rules while retaining distinct receipt and cleanup lease checks. --- .github/pr-automation/current-head.js | 2 +- .github/pr-automation/lifecycle.js | 6 ++---- .github/pr-automation/write-state.js | 18 +++++++----------- 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/.github/pr-automation/current-head.js b/.github/pr-automation/current-head.js index ad82145dd9..eb830e5fc6 100644 --- a/.github/pr-automation/current-head.js +++ b/.github/pr-automation/current-head.js @@ -46,5 +46,5 @@ async function isOpenNonDraftAtHeadNow({ github, owner, repo, pullNumber, expect } module.exports = { - StaleHeadError, assertCurrentHead, isClosedUnmerged, isOpenNonDraftAtHead, isOpenNonDraftAtHeadNow, + StaleHeadError, assertCurrentHead, isClosedUnmerged, isOpenNonDraftAtHeadNow, }; diff --git a/.github/pr-automation/lifecycle.js b/.github/pr-automation/lifecycle.js index 2bad8a81f8..e7a417e696 100644 --- a/.github/pr-automation/lifecycle.js +++ b/.github/pr-automation/lifecycle.js @@ -5,9 +5,7 @@ const { readLatestExactHeadCiRun } = require("./ci-state"); const { canonicalRuns, ownerIsActive, parseLeaseMarker } = require("./automation-lease"); const { parseCheckState } = require("./validate-classifier"); const { trustedReviewOutcome } = require("./review-outcome"); -const { labelsOf } = require("./resolve-state"); - -const ACTOR_LABELS = ["needs-review", "needs-author-action"]; +const { ACTOR_LABELS, labelsOf } = require("./resolve-state"); async function activeLease(github, owner, repo, run, headSha) { const lease = parseLeaseMarker(run?.output?.summary); @@ -120,5 +118,5 @@ async function reconcileLifecycle({ github, owner, repo, prNumber, observedRun = } module.exports = { - ACTOR_LABELS, lifecycleActor, readLifecycleSnapshot, reconcileLifecycle, + lifecycleActor, readLifecycleSnapshot, reconcileLifecycle, }; diff --git a/.github/pr-automation/write-state.js b/.github/pr-automation/write-state.js index ce1f811d03..4be7c30a9d 100644 --- a/.github/pr-automation/write-state.js +++ b/.github/pr-automation/write-state.js @@ -12,7 +12,7 @@ const { hasCurrentClassification, ownsActiveLease, parseLeaseMarker, recheckAutomaticReview, } = require("./automation-lease"); const { - assertCurrentHead, isClosedUnmerged, isOpenNonDraftAtHead, isOpenNonDraftAtHeadNow, + assertCurrentHead, isClosedUnmerged, isOpenNonDraftAtHeadNow, } = require("./current-head"); const { matchesGeneration, readLatestExactHeadCiRun } = require("./ci-state"); const { ACTOR_LABELS, AI_COUNTS, reviewCount } = require("./resolve-state"); @@ -394,15 +394,14 @@ async function writeState({ 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 hasCurrentClassification({ - github, owner, repo, headSha: state.expectedSha, - classificationId: state.admittedGate?.classificationId, requireValid: 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; @@ -415,10 +414,7 @@ async function writeState({ const canPublishReviewPublication = async () => { if (state.forced !== true) return await canPublishReview(); if (!await canMutate()) return false; - return await hasCurrentClassification({ - github, owner, repo, headSha: state.expectedSha, - classificationId: state.admittedGate?.classificationId, requireValid: true, - }); + return await hasForcedClassification(); }; const canPublishReviewReceipt = async () => { if (!await canMutate()) return false;