Skip to content

Commit bdc4df9

Browse files
committed
fix(review): count manual work carried on non-deciding verdicts in the automation rate
queryAutomationRows restricted rows by action IN (merge, close, hold), so a human signal recorded on a non-deciding verdict -- a reevaluation_actor or a reevaluation_reason of maintainer_request riding a label or update_branch row -- never reached buildAutomationRateSeries. The PR's only surviving row was its clean merge, so a pull request a person actually touched was published as automated, inflating the rate. Select every decision_records row in the window and let the fold classify: it already ORs verdictShowsHumanAction across a PR's rows, so seeing the human-signal row is all it needs. The published decided/automated/manual definitions are unchanged -- only which verdicts the fold gets to see. Closes #10013
1 parent d301523 commit bdc4df9

2 files changed

Lines changed: 39 additions & 14 deletions

File tree

src/review/automation-rate.ts

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -41,16 +41,17 @@ import { safeAll } from "./public-stats";
4141
export const AUTOMATION_RATE_PROVENANCE_HORIZON_ISO = "2026-07-29T00:00:00.000Z";
4242

4343
/** The action classes that ENACT a decision. A PR reaching one of these, with no human signal, is what
44-
* "automated" means. Module-private: AUTOMATION_COUNTED_ACTIONS below is the exported surface, and it is
45-
* what both the fold and the read consume -- one definition, rather than a WHERE clause and a predicate
46-
* that can drift apart. */
44+
* "automated" means. The FOLD classifies on this, not the read: the read (#10013) now selects every
45+
* verdict in the window so a human signal carried on a non-deciding verdict is not filtered away before
46+
* the fold ever sees it. */
4747
const AUTOMATION_ENACTING_ACTIONS = ["merge", "close"] as const;
4848

4949
/** The action recording that the gate declined to decide and handed the PR to a person. Module-private for
5050
* the same reason as the set above. */
5151
const AUTOMATION_HOLD_ACTION = "hold";
5252

53-
/** Every action that puts a PR in the series at all -- an enacted decision, or a hold. */
53+
/** Every action that DECIDES a PR -- an enacted decision, or a hold. This is the published set the fold
54+
* keys `decided` on; the read no longer restricts by it (#10013), so it does not gate what rows are seen. */
5455
export const AUTOMATION_COUNTED_ACTIONS: readonly string[] = [...AUTOMATION_ENACTING_ACTIONS, AUTOMATION_HOLD_ACTION];
5556

5657
/** 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<Auto
214215
reevaluation_reason AS reevaluationReason,
215216
reevaluation_actor AS reevaluationActor
216217
FROM decision_records
217-
WHERE created_at >= ?
218-
AND action IN (${AUTOMATION_COUNTED_ACTIONS.map(() => "?").join(", ")})`,
218+
WHERE created_at >= ?`,
219+
// #10013: select EVERY verdict in the window, not just the enacting/hold actions. A human signal
220+
// (reevaluation_actor / reevaluation_reason = 'maintainer_request') can ride a NON-deciding verdict (a
221+
// `label`, an `update_branch`); the old `action IN (...)` filter dropped those rows, so a PR a human
222+
// actually touched read as automated. buildAutomationRateSeries still keys `enacted`/`held` off `action`.
219223
sinceIso,
220-
...AUTOMATION_COUNTED_ACTIONS,
221224
);
222225
}

test/unit/automation-rate.test.ts

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
// would let the rate be inflated.
44
import { describe, expect, it } from "vitest";
55
import {
6-
AUTOMATION_COUNTED_ACTIONS,
76
AUTOMATION_RATE_PROVENANCE_HORIZON_ISO,
87
buildAutomationRateSeries,
98
loadAutomationRateSeries,
@@ -108,6 +107,28 @@ describe("buildAutomationRateSeries", () => {
108107
expect(series.automated).toBe(1);
109108
});
110109

110+
it("counts a PR MANUAL when a human signal rides a NON-deciding verdict, not just a deciding one (#10013)", () => {
111+
// The bug: `queryAutomationRows` restricted rows by `action`, so a human signal carried on a non-deciding
112+
// verdict (a re-labelled PR, a maintainer-triggered re-run recorded as `label`/`update_branch`) never
113+
// reached the fold. The PR's only surviving row was the clean `merge`, so a PR a person actually touched
114+
// read as AUTOMATED. With the filter gone the fold sees the human-signal row and ORs it in.
115+
const humanOnLabel = buildAutomationRateSeries([
116+
row({ pullNumber: 1, action: "label", reevaluationActor: "JSONbored", createdAt: "2026-07-29T10:00:00.000Z" }),
117+
row({ pullNumber: 1, action: "merge", createdAt: "2026-07-29T11:00:00.000Z" }),
118+
]);
119+
expect(humanOnLabel.decided).toBe(1);
120+
expect(humanOnLabel.automated).toBe(0);
121+
expect(humanOnLabel.weeks[0]?.manual).toBe(1);
122+
123+
// Same for a maintainer_request reason riding a non-deciding verdict.
124+
const requestOnUpdate = buildAutomationRateSeries([
125+
row({ pullNumber: 2, action: "update_branch", reevaluationReason: "maintainer_request", createdAt: "2026-07-29T10:00:00.000Z" }),
126+
row({ pullNumber: 2, action: "merge", createdAt: "2026-07-29T11:00:00.000Z" }),
127+
]);
128+
expect(requestOnUpdate.automated).toBe(0);
129+
expect(requestOnUpdate.weeks[0]?.manual).toBe(1);
130+
});
131+
111132
it("counts a hold-only PR as decided-and-manual, never as undecided", () => {
112133
const series = buildAutomationRateSeries([row({ pullNumber: 1, action: "hold" })]);
113134
expect(series.decided).toBe(1);
@@ -217,12 +238,13 @@ describe("loadAutomationRateSeries", () => {
217238
await loadAutomationRateSeries(env);
218239
expect(sql).toContain("FROM decision_records");
219240
expect(sql).toContain("reevaluation_actor");
220-
// The action filter is built FROM the same constant the fold classifies on, so the placeholder count and
221-
// the bind count cannot drift apart -- a mismatch is a D1 error at runtime that no injected-rows test
222-
// above would ever reach.
223-
expect(sql).toContain("action IN (");
224-
expect((sql.match(/\?/g) ?? []).length).toBe(binds.length);
225-
expect(binds.slice(1)).toEqual([...AUTOMATION_COUNTED_ACTIONS]);
241+
// #10013: the read must NOT restrict rows by action -- a human signal (reevaluation_actor /
242+
// reevaluation_reason='maintainer_request') can ride a non-deciding verdict, and the old `action IN (...)`
243+
// filter dropped exactly those rows, undercounting manual work. The only bind is the `created_at >= ?`
244+
// window bound: one placeholder, one bind, and no action list.
245+
expect(sql).not.toContain("action IN (");
246+
expect((sql.match(/\?/g) ?? []).length).toBe(1);
247+
expect(binds).toEqual([expect.any(String)]);
226248
});
227249

228250
it("degrades to an empty series when the read throws, never failing the stats endpoint", async () => {

0 commit comments

Comments
 (0)