fix(setup): let the generated workflow's jobs actually start

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

src/setup.tsmodified+18-1

Changes to src/setup.ts

		⋯1,618 linesShow the 1,618 lines above this hunk@@ -1619,6 +1619,7 @@ jobs:
1619	1619	run_review: \${{ steps.determine.outputs.run_review }}
1620	1620	run_fixci: \${{ steps.determine.outputs.run_fixci }}
1621	1621	run_report: \${{ steps.determine.outputs.run_report }}
	1622	+ run_any: \${{ steps.determine.outputs.run_any }}
1622	1623	steps:
1623	1624	- name: Determine which jobs to run
1624	1625	id: determine
		⋯16 linesShow the 16 lines above this hunk@@ -1641,6 +1642,7 @@ jobs:
1641	1642	echo "run_command=false" >> \$GITHUB_OUTPUT
1642	1643	echo "run_review=false" >> \$GITHUB_OUTPUT
1643	1644	echo "run_fixci=false" >> \$GITHUB_OUTPUT
	1645	+ echo "run_report=false" >> \$GITHUB_OUTPUT
1644	1646
1645	1647	if [ "\$" = "pull_request" ] && [ "\$" != "edited" ]; then
1646	1648	# Opened, marked ready, or pushed to — review it. Draft handling and
		⋯76 linesShow the 76 lines above this hunk@@ -1723,12 +1725,27 @@ jobs:
1723	1725	${renderScheduleDispatch(plan)}
1724	1726	fi
1725	1727
	1728	+ # Every job below needs \`setup\`, and a job whose \`needs\` was skipped
	1729	+ # is skipped with it — so \`setup\` has to run whenever anything does.
	1730	+ # Derived from what was just written rather than restated, because
	1731	+ # restating it is how review, command and fix-ci came to be
	1732	+ # unreachable: each was added with its own output and nobody added it
	1733	+ # to setup's condition.
	1734	+ if grep -q '=true\$' "\$GITHUB_OUTPUT"; then
	1735	+ echo "run_any=true" >> \$GITHUB_OUTPUT
	1736	+ else
	1737	+ echo "run_any=false" >> \$GITHUB_OUTPUT
	1738	+ fi
	1739	+
1726	1740	# Shared setup job for common dependencies
1727	1741	setup:
1728	1742	runs-on: ubuntu-latest
1729	1743	timeout-minutes: 10
1730	1744	needs: determine-jobs
1731		- if: \${{ needs.determine-jobs.outputs.run_check == 'true' || needs.determine-jobs.outputs.run_update == 'true' || needs.determine-jobs.outputs.run_dashboard == 'true' || needs.determine-jobs.outputs.run_report == 'true' }}
	1745	+ # \`gate\`, \`touch\`, \`post-merge\` and \`issue-links\` gate on \`github.event_name\`
	1746	+ # rather than on an output, so those two events are named here instead of
	1747	+ # having their conditions restated a second time and left to drift.
	1748	+ if: \${{ needs.determine-jobs.outputs.run_any == 'true' || github.event_name == 'pull_request' || github.event_name == 'issues' }}
1732	1749	steps:
1733	1750	- name: Checkout repository
1734	1751	uses: actions/checkout@v4

test/workflow-presets.test.tsmodified+6-2

Changes to test/workflow-presets.test.ts

		⋯115 linesShow the 115 lines above this hunk@@ -116,8 +116,12 @@ describe('workflow presets', () => {
116	116
117	117	it('success case - its setup dependency runs on a report-only tick', () => {
118	118	// `report` needs `setup`, and a skipped dependency skips the job that
119		- // needs it. Gating setup on the update jobs alone was the whole bug.
120		- expect(parse('standard').jobs.setup.if).toContain('run_report')
	119	+ // needs it. Asserted through the derived flag rather than by looking for
	120	+ // `run_report` in setup's condition — naming one output there is what
	121	+ // left the other seven jobs unreachable. See
	122	+ // test/workflow-reachability.test.ts for the per-event proof.
	123	+ expect(parse('standard').jobs.setup.if).toContain('run_any')
	124	+ expect(parse('standard').jobs['determine-jobs'].outputs).toHaveProperty('run_any')
121	125	})
122	126	})
123	127

test/workflow-reachability.test.tsadded+200-0

Changes to test/workflow-reachability.test.ts

		@@ -0,0 +1,200 @@
	1	+import { describe, expect, it } from 'bun:test'
	2	+import { generateUnifiedWorkflow } from '../src/setup'
	3	+
	4	+interface Job { if?: string, needs?: string | string[] }
	5	+interface Workflow { jobs: Record<string, Job> }
	6	+
	7	+const workflow = Bun.YAML.parse(generateUnifiedWorkflow(false)) as Workflow
	8	+
	9	+/**
	10	+ * Evaluate the subset of GitHub's expression syntax this workflow uses.
	11	+ *
	12	+ * Rewritten to JavaScript rather than interpreted, because the operators in
	13	+ * play — `==`, `!=`, `&&`, `||` — mean the same thing in both, and the only
	14	+ * real differences are `always()` and the hyphen in `determine-jobs`, which
	15	+ * JavaScript cannot address with a dot.
	16	+ */
	17	+function evaluate(expression: string | undefined, ctx: Record<string, unknown>): boolean {
	18	+ if (!expression)
	19	+ return true
	20	+
	21	+ const body = expression
	22	+ .replace(/^\$\{\{/, '')
	23	+ .replace(/\}\}$/, '')
	24	+ .replace(/always\(\)/g, 'true')
	25	+ .replace(/!cancelled\(\)/g, 'true')
	26	+ .replace(/needs\.determine-jobs\./g, 'c.needs["determine-jobs"].')
	27	+ .replace(/(?<!c\.)\bneeds\./g, 'c.needs.')
	28	+ .replace(/(?<!c\.)\bgithub\./g, 'c.github.')
	29	+
	30	+ // eslint-disable-next-line no-new-func
	31	+ return Boolean(new Function('c', `return (${body})`)(ctx))
	32	+}
	33	+
	34	+/**
	35	+ * What `determine-jobs` decides for an event, and what `setup` then does.
	36	+ *
	37	+ * The outputs are stated per event rather than read out of the shell script:
	38	+ * the bug this guards against is not in what the router decides, it is in
	39	+ * whether the jobs it decided for can actually start.
	40	+ */
	41	+function context(github: Record<string, unknown>, outputs: Record<string, string>) {
	42	+ const full = {
	43	+ run_check: 'false',
	44	+ run_update: 'false',
	45	+ run_dashboard: 'false',
	46	+ run_command: 'false',
	47	+ run_review: 'false',
	48	+ run_fixci: 'false',
	49	+ run_report: 'false',
	50	+ ...outputs,
	51	+ }
	52	+
	53	+ const ctx = {
	54	+ github,
	55	+ needs: {
	56	+ 'determine-jobs': {
	57	+ outputs: { ...full, run_any: Object.values(full).includes('true') ? 'true' : 'false' },
	58	+ result: 'success',
	59	+ },
	60	+ 'setup': { result: 'success' },
	61	+ },
	62	+ }
	63	+
	64	+ return ctx
	65	+}
	66	+
	67	+/** Whether a job actually starts: its own gate, and every job it needs. */
	68	+function runs(name: string, ctx: Record<string, unknown>): boolean {
	69	+ const job = workflow.jobs[name]
	70	+ const needs = job.needs ? [job.needs].flat() : []
	71	+ const gated = evaluate(job.if, ctx)
	72	+
	73	+ // A job whose dependency was skipped is skipped with it, unless its own
	74	+ // condition contains a status function. That is the whole defect.
	75	+ const skipsWithNeeds = !/always\(\)|!cancelled\(\)/.test(job.if ?? '')
	76	+ const needsRun = needs.every(dep => dep === 'determine-jobs' || runs(dep, ctx))
	77	+
	78	+ return gated && (needsRun || !skipsWithNeeds)
	79	+}
	80	+
	81	+/**
	82	+ * `setup` is a dependency of every job in the generated workflow, and a job
	83	+ * whose `needs` was skipped is skipped with it. Its condition named four of
	84	+ * the eleven gates, so the seven it did not name could never start — the
	85	+ * review, the @buddy commands, CI repair, the pre-merge gate, the finishing
	86	+ * touches, the post-merge step and the issue quick-links.
	87	+ *
	88	+ * #1412 fixed exactly this for the health report and did not check its
	89	+ * siblings. This suite is what would have caught that.
	90	+ */
	91	+describe('generated workflow reachability', () => {
	92	+ const cases: Array<{ event: string, github: Record<string, unknown>, outputs: Record<string, string>, expect: string[] }> = [
	93	+ {
	94	+ event: 'pull request opened',
	95	+ github: { event_name: 'pull_request', event: { action: 'opened', pull_request: { draft: false, merged: false } } },
	96	+ outputs: { run_review: 'true' },
	97	+ expect: ['review', 'gate'],
	98	+ },
	99	+ {
	100	+ event: 'pull request synchronized',
	101	+ github: { event_name: 'pull_request', event: { action: 'synchronize', pull_request: { draft: false, merged: false } } },
	102	+ outputs: { run_review: 'true' },
	103	+ expect: ['review', 'gate'],
	104	+ },
	105	+ {
	106	+ event: 'buddy pull request edited',
	107	+ github: { event_name: 'pull_request', event: { action: 'edited', pull_request: { draft: false, merged: false } } },
	108	+ outputs: { run_check: 'true' },
	109	+ expect: ['check', 'touch', 'gate'],
	110	+ },
	111	+ {
	112	+ event: 'pull request merged',
	113	+ github: { event_name: 'pull_request', event: { action: 'closed', pull_request: { draft: false, merged: true } } },
	114	+ outputs: {},
	115	+ expect: ['post-merge'],
	116	+ },
	117	+ {
	118	+ event: '@buddy comment',
	119	+ github: { event_name: 'issue_comment', event: { action: 'created' } },
	120	+ outputs: { run_command: 'true' },
	121	+ expect: ['command'],
	122	+ },
	123	+ {
	124	+ event: 'failing run on a buddy branch',
	125	+ github: { event_name: 'workflow_run', event: { action: 'completed', workflow_run: { conclusion: 'failure' } } },
	126	+ outputs: { run_fixci: 'true' },
	127	+ expect: ['fix-ci'],
	128	+ },
	129	+ {
	130	+ event: 'issue opened',
	131	+ github: { event_name: 'issues', event: { action: 'opened' } },
	132	+ outputs: {},
	133	+ expect: ['issue-links'],
	134	+ },
	135	+ {
	136	+ event: 'scheduled update',
	137	+ github: { event_name: 'schedule', event: { schedule: '0 9 * * 1,3,5' } },
	138	+ outputs: { run_update: 'true', run_dashboard: 'true' },
	139	+ expect: ['dependency-update', 'dashboard-update'],
	140	+ },
	141	+ {
	142	+ event: 'weekly health report',
	143	+ github: { event_name: 'schedule', event: { schedule: '0 9 * * 1' } },
	144	+ outputs: { run_report: 'true' },
	145	+ expect: ['report'],
	146	+ },
	147	+ ]
	148	+
	149	+ for (const testCase of cases) {
	150	+ it(`success case - ${testCase.event} reaches ${testCase.expect.join(', ')}`, () => {
	151	+ const ctx = context(testCase.github, testCase.outputs)
	152	+
	153	+ // Named first: everything downstream depends on it, and it is where the
	154	+ // whole family of jobs was being lost.
	155	+ expect(runs('setup', ctx)).toBe(true)
	156	+
	157	+ for (const job of testCase.expect)
	158	+ expect({ job, runs: runs(job, ctx) }).toEqual({ job, runs: true })
	159	+ })
	160	+ }
	161	+
	162	+ it('failure case - ordinary discussion starts nothing', () => {
	163	+ // The workflow triggers on every comment created. A comment that does not
	164	+ // mention the bot must not pay for a checkout and an install.
	165	+ const ctx = context(
	166	+ { event_name: 'issue_comment', event: { action: 'created' } },
	167	+ {},
	168	+ )
	169	+
	170	+ expect(runs('setup', ctx)).toBe(false)
	171	+ expect(runs('command', ctx)).toBe(false)
	172	+ })
	173	+
	174	+ it('failure case - a green run starts nothing', () => {
	175	+ const ctx = context(
	176	+ { event_name: 'workflow_run', event: { action: 'completed', workflow_run: { conclusion: 'success' } } },
	177	+ {},
	178	+ )
	179	+
	180	+ expect(runs('setup', ctx)).toBe(false)
	181	+ expect(runs('fix-ci', ctx)).toBe(false)
	182	+ })
	183	+
	184	+ it('success case - every job that needs setup is reachable on some event', () => {
	185	+ // The structural version of the same claim: a job nothing can ever start
	186	+ // is a job that should not have been generated.
	187	+ const dependents = Object.entries(workflow.jobs)
	188	+ .filter(([, job]) => [job.needs ?? []].flat().includes('setup'))
	189	+ .map(([name]) => name)
	190	+
	191	+ const reachable = new Set(
	192	+ cases.flatMap(testCase => {
	193	+ const ctx = context(testCase.github, testCase.outputs)
	194	+ return dependents.filter(name => runs(name, ctx))
	195	+ }),
	196	+ )
	197	+
	198	+ expect([...dependents].filter(name => !reachable.has(name))).toEqual([])
	199	+ })
	200	+})