ReviewOS

also looking at this

stacks/buddy

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

#1418
Merged glennmichael123 wants to merge fix/workflow-reachability into main
3 files +224 -3
src/setup.tsmodified+18-1
Changes to src/setup.ts
@@ -1619,6 +1619,7 @@ jobs:
16191619 run_review: \${{ steps.determine.outputs.run_review }}
16201620 run_fixci: \${{ steps.determine.outputs.run_fixci }}
16211621 run_report: \${{ steps.determine.outputs.run_report }}
1622 run_any: \${{ steps.determine.outputs.run_any }}
16221623 steps:
16231624 - name: Determine which jobs to run
16241625 id: determine
@@ -1641,6 +1642,7 @@ jobs:
16411642 echo "run_command=false" >> \$GITHUB_OUTPUT
16421643 echo "run_review=false" >> \$GITHUB_OUTPUT
16431644 echo "run_fixci=false" >> \$GITHUB_OUTPUT
1645 echo "run_report=false" >> \$GITHUB_OUTPUT
16441646
16451647 if [ "\$" = "pull_request" ] && [ "\$" != "edited" ]; then
16461648 # Opened, marked ready, or pushed to — review it. Draft handling and
@@ -1723,12 +1725,27 @@ jobs:
17231725${renderScheduleDispatch(plan)}
17241726 fi
17251727
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
17261740 # Shared setup job for common dependencies
17271741 setup:
17281742 runs-on: ubuntu-latest
17291743 timeout-minutes: 10
17301744 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' }}
17321749 steps:
17331750 - name: Checkout repository
17341751 uses: actions/checkout@v4
test/workflow-presets.test.tsmodified+6-2
Changes to test/workflow-presets.test.ts
@@ -116,8 +116,12 @@ describe('workflow presets', () => {
116116
117117 it('success case - its setup dependency runs on a report-only tick', () => {
118118 // `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')
121125 })
122126 })
123127
test/workflow-reachability.test.tsadded+200-0
Changes to test/workflow-reachability.test.ts
@@ -0,0 +1,200 @@
1import { describe, expect, it } from 'bun:test'
2import { generateUnifiedWorkflow } from '../src/setup'
3
4interface Job { if?: string, needs?: string | string[] }
5interface Workflow { jobs: Record<string, Job> }
6
7const 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 */
17function 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 */
41function 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. */
68function 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 */
91describe('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})