fix(api): ?branch= silently hid nearly every workflow run #5600
4 changed files+85−25
Modifiedsrc/__tests__/actions-runs-list.test.ts+42−14View fileUnifiedSplit
@@ -16,26 +16,53 @@
1616import { describe, expect, test } from "bun:test";
1717import { parseRunListQuery } from "../routes/api-v2";
1818
19describe("branch accepts what both kinds of client actually send", () => {
20 test("a short name becomes a full ref", () => {
21 expect(parseRunListQuery({ branch: "main" }).ref).toBe("refs/heads/main");
19describe("branch matches every form the column actually holds", () => {
20 /**
21 * `workflow_runs.ref` contains BOTH `main` and `refs/heads/main`.
22 * push-workflow-sync.ts and pr-workflow-sync.ts write the bare branch;
23 * pr-slash-commands.ts and scheduled-workflows.ts write the qualified ref.
24 * Two writers, two formats, one column.
25 *
26 * The first version of this endpoint normalised the query to one form and
27 * compared. That is the obvious implementation and it was wrong:
28 * `?branch=main` returned a single run from four days earlier while the
29 * unfiltered list showed six from the last hour, because it silently
30 * excluded every push-triggered run — nearly all of them. A filter that
31 * returns a plausible-looking short list is worse than one that errors,
32 * because nobody checks it.
33 */
34 test("a short name matches the bare ref AND the qualified one", () => {
35 expect(parseRunListQuery({ branch: "main" }).refs.sort()).toEqual([
36 "main",
37 "refs/heads/main",
38 ]);
2239 });
2340
24 test("an already-qualified ref is left alone", () => {
25 // Clients written against git send this form. Prefixing it again would
26 // produce refs/heads/refs/heads/main and silently match nothing — an
27 // empty list, which reads as "no runs on that branch" rather than as the
28 // bug it is.
29 expect(parseRunListQuery({ branch: "refs/heads/main" }).ref).toBe("refs/heads/main");
41 test("a qualified ref also matches the bare form", () => {
42 // Clients written against git send this form; the rows they are looking
43 // for were mostly written by the push path in the other form.
44 expect(parseRunListQuery({ branch: "refs/heads/main" }).refs.sort()).toEqual([
45 "main",
46 "refs/heads/main",
47 ]);
3048 });
3149
32 test("a tag ref survives", () => {
33 expect(parseRunListQuery({ branch: "refs/tags/v1.2.0" }).ref).toBe("refs/tags/v1.2.0");
50 test("a qualified ref is never double-prefixed", () => {
51 // refs/heads/refs/heads/main matches nothing and reads as "no runs".
52 for (const r of parseRunListQuery({ branch: "refs/heads/main" }).refs) {
53 expect(r).not.toContain("refs/heads/refs/");
54 }
55 });
56
57 test("a tag ref survives and is not mangled into a branch", () => {
58 const refs = parseRunListQuery({ branch: "refs/tags/v1.2.0" }).refs;
59 expect(refs).toContain("refs/tags/v1.2.0");
60 expect(refs.join(" ")).not.toContain("refs/heads/refs/tags");
3461 });
3562
3663 test("absent or blank means no branch filter, not a filter on empty", () => {
37 expect(parseRunListQuery({}).ref).toBeNull();
38 expect(parseRunListQuery({ branch: " " }).ref).toBeNull();
64 expect(parseRunListQuery({}).refs).toEqual([]);
65 expect(parseRunListQuery({ branch: " " }).refs).toEqual([]);
3966 });
4067});
4168
@@ -84,7 +111,8 @@ describe("the other filters", () => {
84111
85112 test("no query at all filters on nothing", () => {
86113 const f = parseRunListQuery({});
87 expect([f.ref, f.headSha, f.event, f.status]).toEqual([null, null, null, null]);
114 expect(f.refs).toEqual([]);
115 expect([f.headSha, f.event, f.status]).toEqual([null, null, null]);
88116 expect(f.perPage).toBe(30);
89117 expect(f.page).toBe(1);
90118 });
Modifiedsrc/lib/pr-workflow-sync.ts+2−0View fileUnifiedSplit
@@ -91,6 +91,8 @@ export async function enqueuePullRequestWorkflows(
9191 workflowId: row.id,
9292 repositoryId: opts.repositoryId,
9393 event: "pull_request",
94 // Bare branch name — same column, same divergence as
95 // push-workflow-sync.ts. See the note there.
9496 ref: opts.headBranch,
9597 commitSha: opts.headSha,
9698 triggeredBy: opts.triggeredBy ?? null,
Modifiedsrc/lib/push-workflow-sync.ts+8−0View fileUnifiedSplit
@@ -158,6 +158,14 @@ export async function syncAndEnqueuePushWorkflows(
158158 workflowId: row.id,
159159 repositoryId: opts.repositoryId,
160160 event: "push",
161 // BARE branch name, not `refs/heads/...`. Other enqueue sites
162 // (pr-slash-commands.ts, scheduled-workflows.ts) write the qualified
163 // form into this same column, so `workflow_runs.ref` holds both.
164 // Readers must match BOTH — see parseRunListQuery in api-v2.ts, where
165 // assuming one form silently hid every push-triggered run from the
166 // branch filter. Do not "tidy" this to one format without converting
167 // the existing rows and auditing every reader; the divergence is
168 // survivable, a half-migration is not.
161169 ref: opts.branch,
162170 commitSha: opts.commitSha,
163171 triggeredBy: opts.triggeredBy ?? null,
Modifiedsrc/routes/api-v2.ts+33−11View fileUnifiedSplit
@@ -3018,11 +3018,11 @@ apiv2.get(
30183018
30193019 const conditions = [eq(workflowRuns.workflowId, workflowRow.id)];
30203020 if (branch) {
3021 // Accept either short branch name or fully-qualified refs/heads/...
3022 const refValue = branch.startsWith("refs/")
3023 ? branch
3024 : `refs/heads/${branch}`;
3025 conditions.push(eq(workflowRuns.ref, refValue));
3021 // Same both-forms match as the repo-wide list above — the ref column
3022 // holds `main` from some writers and `refs/heads/main` from others, so
3023 // comparing against one form silently drops the other.
3024 const { refs } = parseRunListQuery({ branch });
3025 conditions.push(or(...refs.map((r) => eq(workflowRuns.ref, r)))!);
30263026 }
30273027 if (headSha) conditions.push(eq(workflowRuns.commitSha, headSha));
30283028
@@ -3089,7 +3089,13 @@ export function parseRunListQuery(q: {
30893089}): {
30903090 perPage: number;
30913091 page: number;
3092 ref: string | null;
3092 /**
3093 * Every stored form the requested branch could have. BOTH are needed:
3094 * `workflow_runs.ref` holds `main` from push-workflow-sync.ts and
3095 * pr-workflow-sync.ts, and `refs/heads/main` from pr-slash-commands.ts and
3096 * scheduled-workflows.ts. Two writers, two formats, one column.
3097 */
3098 refs: string[];
30933099 headSha: string | null;
30943100 event: string | null;
30953101 status: string | null;
@@ -3100,10 +3106,22 @@ export function parseRunListQuery(q: {
31003106 return {
31013107 perPage,
31023108 page,
3103 // Accept a short branch name or a fully-qualified ref, because clients
3104 // written against `git` send one and clients written against the API
3105 // send the other, and refusing either is a support ticket.
3106 ref: branch ? (branch.startsWith("refs/") ? branch : `refs/heads/${branch}`) : null,
3109 // Accept a short branch name or a fully-qualified ref from the CLIENT,
3110 // because clients written against `git` send one and clients written
3111 // against the API send the other — and then match BOTH forms in the
3112 // database, because the column genuinely contains both.
3113 //
3114 // Normalising to one form and comparing was the obvious implementation
3115 // and it was wrong: `?branch=main` returned a single run from four days
3116 // earlier while the unfiltered list showed six from the last hour. It
3117 // silently excluded every push-triggered run, which is nearly all of
3118 // them. A filter that returns a plausible-looking short list is worse
3119 // than one that errors, because nobody checks it.
3120 refs: branch
3121 ? branch.startsWith("refs/")
3122 ? [branch, branch.replace(/^refs\/heads\//, "")]
3123 : [branch, `refs/heads/${branch}`]
3124 : [],
31073125 headSha: (q.head_sha || "").trim() || null,
31083126 event: (q.event || "").trim() || null,
31093127 status: (q.status || "").trim() || null,
@@ -3141,7 +3159,11 @@ apiv2.get("/repos/:owner/:repo/actions/runs", async (c) => {
31413159 const offset = (f.page - 1) * f.perPage;
31423160
31433161 const conditions = [eq(workflowRuns.repositoryId, repoRow.id)];
3144 if (f.ref) conditions.push(eq(workflowRuns.ref, f.ref));
3162 if (f.refs.length > 0) {
3163 conditions.push(
3164 or(...f.refs.map((r) => eq(workflowRuns.ref, r)))!
3165 );
3166 }
31453167 if (f.headSha) conditions.push(eq(workflowRuns.commitSha, f.headSha));
31463168 if (f.event) conditions.push(eq(workflowRuns.event, f.event));
31473169 if (f.status) {
31483170
c comment · e edit title · m merge · a approve · r request changes · ? shortcuts