diff --git a/src/review/automation-rate.ts b/src/review/automation-rate.ts index 62cb4cdeb..99e63f39a 100644 --- a/src/review/automation-rate.ts +++ b/src/review/automation-rate.ts @@ -41,16 +41,17 @@ import { safeAll } from "./public-stats"; export const AUTOMATION_RATE_PROVENANCE_HORIZON_ISO = "2026-07-29T00:00:00.000Z"; /** The action classes that ENACT a decision. A PR reaching one of these, with no human signal, is what - * "automated" means. Module-private: AUTOMATION_COUNTED_ACTIONS below is the exported surface, and it is - * what both the fold and the read consume -- one definition, rather than a WHERE clause and a predicate - * that can drift apart. */ + * "automated" means. The FOLD classifies on this, not the read: the read (#10013) now selects every + * verdict in the window so a human signal carried on a non-deciding verdict is not filtered away before + * the fold ever sees it. */ const AUTOMATION_ENACTING_ACTIONS = ["merge", "close"] as const; /** The action recording that the gate declined to decide and handed the PR to a person. Module-private for * the same reason as the set above. */ const AUTOMATION_HOLD_ACTION = "hold"; -/** Every action that puts a PR in the series at all -- an enacted decision, or a hold. */ +/** Every action that DECIDES a PR -- an enacted decision, or a hold. This is the published set the fold + * keys `decided` on; the read no longer restricts by it (#10013), so it does not gate what rows are seen. */ export const AUTOMATION_COUNTED_ACTIONS: readonly string[] = [...AUTOMATION_ENACTING_ACTIONS, AUTOMATION_HOLD_ACTION]; /** How completely a week could be measured. `full` weeks see every manual signal; `holds_only` weeks predate @@ -214,9 +215,11 @@ async function queryAutomationRows(env: unknown, sinceIso: string): Promise= ? - AND action IN (${AUTOMATION_COUNTED_ACTIONS.map(() => "?").join(", ")})`, + WHERE created_at >= ?`, + // #10013: select EVERY verdict in the window, not just the enacting/hold actions. A human signal + // (reevaluation_actor / reevaluation_reason = 'maintainer_request') can ride a NON-deciding verdict (a + // `label`, an `update_branch`); the old `action IN (...)` filter dropped those rows, so a PR a human + // actually touched read as automated. buildAutomationRateSeries still keys `enacted`/`held` off `action`. sinceIso, - ...AUTOMATION_COUNTED_ACTIONS, ); } diff --git a/test/unit/automation-rate.test.ts b/test/unit/automation-rate.test.ts index a8cbcdadb..5cd146fdc 100644 --- a/test/unit/automation-rate.test.ts +++ b/test/unit/automation-rate.test.ts @@ -3,7 +3,6 @@ // would let the rate be inflated. import { describe, expect, it } from "vitest"; import { - AUTOMATION_COUNTED_ACTIONS, AUTOMATION_RATE_PROVENANCE_HORIZON_ISO, buildAutomationRateSeries, loadAutomationRateSeries, @@ -108,6 +107,28 @@ describe("buildAutomationRateSeries", () => { expect(series.automated).toBe(1); }); + it("counts a PR MANUAL when a human signal rides a NON-deciding verdict, not just a deciding one (#10013)", () => { + // The bug: `queryAutomationRows` restricted rows by `action`, so a human signal carried on a non-deciding + // verdict (a re-labelled PR, a maintainer-triggered re-run recorded as `label`/`update_branch`) never + // reached the fold. The PR's only surviving row was the clean `merge`, so a PR a person actually touched + // read as AUTOMATED. With the filter gone the fold sees the human-signal row and ORs it in. + const humanOnLabel = buildAutomationRateSeries([ + row({ pullNumber: 1, action: "label", reevaluationActor: "JSONbored", createdAt: "2026-07-29T10:00:00.000Z" }), + row({ pullNumber: 1, action: "merge", createdAt: "2026-07-29T11:00:00.000Z" }), + ]); + expect(humanOnLabel.decided).toBe(1); + expect(humanOnLabel.automated).toBe(0); + expect(humanOnLabel.weeks[0]?.manual).toBe(1); + + // Same for a maintainer_request reason riding a non-deciding verdict. + const requestOnUpdate = buildAutomationRateSeries([ + row({ pullNumber: 2, action: "update_branch", reevaluationReason: "maintainer_request", createdAt: "2026-07-29T10:00:00.000Z" }), + row({ pullNumber: 2, action: "merge", createdAt: "2026-07-29T11:00:00.000Z" }), + ]); + expect(requestOnUpdate.automated).toBe(0); + expect(requestOnUpdate.weeks[0]?.manual).toBe(1); + }); + it("counts a hold-only PR as decided-and-manual, never as undecided", () => { const series = buildAutomationRateSeries([row({ pullNumber: 1, action: "hold" })]); expect(series.decided).toBe(1); @@ -217,12 +238,13 @@ describe("loadAutomationRateSeries", () => { await loadAutomationRateSeries(env); expect(sql).toContain("FROM decision_records"); expect(sql).toContain("reevaluation_actor"); - // The action filter is built FROM the same constant the fold classifies on, so the placeholder count and - // the bind count cannot drift apart -- a mismatch is a D1 error at runtime that no injected-rows test - // above would ever reach. - expect(sql).toContain("action IN ("); - expect((sql.match(/\?/g) ?? []).length).toBe(binds.length); - expect(binds.slice(1)).toEqual([...AUTOMATION_COUNTED_ACTIONS]); + // #10013: the read must NOT restrict rows by action -- a human signal (reevaluation_actor / + // reevaluation_reason='maintainer_request') can ride a non-deciding verdict, and the old `action IN (...)` + // filter dropped exactly those rows, undercounting manual work. The only bind is the `created_at >= ?` + // window bound: one placeholder, one bind, and no action list. + expect(sql).not.toContain("action IN ("); + expect((sql.match(/\?/g) ?? []).length).toBe(1); + expect(binds).toEqual([expect.any(String)]); }); it("degrades to an empty series when the read throws, never failing the stats endpoint", async () => {