Visitar URL original
Name Each Job That Posts the Required Check Off a Pull Request Run (#… · ptr727/ProjectTemplate@23c7d78 · GitHub
Skip to content

Commit 23c7d78

Browse files
ptr727claude
ptr727
and
claude
authored
Name Each Job That Posts the Required Check Off a Pull Request Run (#3799)
## Summary In `spec/audit.py`, `check_interface` skipped the D1.5 every-event finding for the whole workflow once any one job used the pull_request-only name expression. A second job could still post the required check on a non-PR run, and nothing flagged it. The check now reads each job's `name:` together with its job key. A job's name off a pull_request run is its literal name, or the `||` fallback string when the name uses the pull_request-only expression. Some workflows are triggered by events other than `pull_request`. In those, every job whose off-PR name equals the required check gets its own DRIFT, and the finding names that job. The other jobs no longer affect it. The existing missing-name DRIFT keeps its develop behaviour. It covers names that are neither a literal nor the pull_request-only expression, and it still flags an expression whose two branches are both the required name on a pull_request-only workflow. It now runs alongside the per-job check rather than ahead of it. - **A job falling back to the required name, with no gated job.** It reports both findings, and both are true. The PR run never posts the check, and a non-PR run does. - **An expression whose two branches are both the required name, on a dispatch-triggered workflow.** It reports only its per-job finding. That job does post the check on the PR run, so the missing-name finding would be false there. Repro on a constructed stub triggered by `pull_request` and `workflow_dispatch`: - **A literal sibling.** One job is named by the D1.5 expression, and a second carries the literal `Check pull request workflow status job`. This returned `[]` before the change. It now returns one DRIFT naming the second job. - **A fallback sibling.** The second job is named `${{ github.event_name == 'pull_request' && 'Other job' || 'Check pull request workflow status job' }}`. This also returned `[]` before the change. It now returns one DRIFT naming that job. Unchanged: a gated job alone raises no finding. A literal name on a pull_request-only workflow raises no finding either. ## Tests New `INTERFACE_CASES` rows in `tests/test_audit_helpers.py`: - A literal sibling beside a gated job. - A fallback sibling beside a gated job. - Two literal jobs, one finding each. - A fallback-only job, both findings. - An every-event expression on a dispatch-triggered workflow, one finding. - A fallback job beside an every-event expression naming another check, both findings. Two new tests assert which job each finding names, and that the dispatch every-event case reports its per-job finding rather than the generic one. Run against develop's `spec/audit.py`, every new row fails except the dispatch every-event row. That row keeps develop's count, so its message test is what pins it. The last row also fails if the suppression stops checking for the required name. `spec/audit.py --selftest` and the full `OPERATIONS.md` "Local Verification" block pass. Closes on promotion: #3334 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent a8346fe commit 23c7d78

2 files changed

Lines changed: 112 additions & 13 deletions

File tree

‎spec/audit.py‎

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -972,29 +972,40 @@ def check_interface(path, contract, text):
972972
if k not in jobs:
973973
findings.append(("DRIFT", f"interface: {path} missing required job '{k}'"))
974974
name = contract.get("requiredCheckName")
975-
nodes = (_mapping(job).get("name") for job in jobs.values())
976-
values = [n.value for n in nodes if isinstance(n, yaml.ScalarNode)]
977-
gated = {
978-
m[1]
979-
for v in values
980-
if (m := PR_ONLY_NAME.fullmatch(_strip_unquoted_space(v))) and m[1] != m[2]
975+
names = {
976+
key: node.value
977+
for key, job in jobs.items()
978+
if isinstance(node := _mapping(job).get("name"), yaml.ScalarNode)
981979
}
982-
if name and name not in gated and name not in values:
980+
matches = {
981+
key: m
982+
for key, v in names.items()
983+
if (m := PR_ONLY_NAME.fullmatch(_strip_unquoted_space(v)))
984+
}
985+
gated = {m[1] for m in matches.values() if m[1] != m[2]}
986+
# Each job is its own finding, since neither a gated job nor a missing one excuses another yielding the name off a pull_request run.
987+
events = workflow_events(_plain(root) if root is not None else None)
988+
off_pr = [
989+
key
990+
for key, value in names.items()
991+
if name
992+
and events - {"pull_request"}
993+
and (matches[key][2] if key in matches else value) == name
994+
]
995+
# An expression naming the check on every event is already its job's off-PR finding where another event triggers the workflow.
996+
every_event = any(m[1] == m[2] == name for m in matches.values())
997+
if name and name not in gated and name not in names.values() and not (off_pr and every_event):
983998
findings.append(
984999
(
9851000
"DRIFT",
9861001
f"interface: {path} names the ruleset-bound check '{name}' neither as a literal job name nor through the pull_request-only expression WORKFLOW.md D1.5 gives",
9871002
)
9881003
)
989-
elif (
990-
name
991-
and name not in gated
992-
and workflow_events(_plain(root) if root is not None else None) - {"pull_request"}
993-
):
1004+
for key in off_pr:
9941005
findings.append(
9951006
(
9961007
"DRIFT",
997-
f"interface: {path} names its job '{name}' on every event, so a run the pull request did not start can post the required check on its head (WORKFLOW.md D1.5)",
1008+
f"interface: {path} job '{key}' is named '{name}' on a run other than pull_request, so a run the pull request did not start can post the required check on its head (WORKFLOW.md D1.5)",
9981009
)
9991010
)
10001011
tok = contract.get("artifactNameToken")

‎tests/test_audit_helpers.py‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,71 @@ def tearDownModule() -> None:
183183
PR_CONTRACT,
184184
1,
185185
),
186+
(
187+
"a literal check name beside a gated one, on a workflow a dispatch also triggers",
188+
PR_HEAD.replace("on: pull_request\n", "on:\n pull_request:\n workflow_dispatch:\n")
189+
+ PR_CHECK.replace(PR_CHECK_NAME, PR_ONLY_EXPRESSION)
190+
+ PR_CHECK.replace("check-workflow-status:", "extra:"),
191+
PR_CONTRACT,
192+
1,
193+
),
194+
(
195+
"two literal check names on a workflow a dispatch also triggers, one finding each",
196+
PR_HEAD.replace("on: pull_request\n", "on:\n pull_request:\n workflow_dispatch:\n")
197+
+ PR_CHECK
198+
+ PR_CHECK.replace("check-workflow-status:", "extra:"),
199+
PR_CONTRACT,
200+
2,
201+
),
202+
(
203+
"an expression falling back to the check name beside a gated one",
204+
PR_HEAD.replace("on: pull_request\n", "on:\n pull_request:\n workflow_dispatch:\n")
205+
+ PR_CHECK.replace(PR_CHECK_NAME, PR_ONLY_EXPRESSION)
206+
+ PR_CHECK.replace("check-workflow-status:", "extra:").replace(
207+
PR_CHECK_NAME,
208+
"${{ github.event_name == 'pull_request' && 'Other job' || '" + PR_CHECK_NAME + "' }}",
209+
),
210+
PR_CONTRACT,
211+
1,
212+
),
213+
(
214+
"an expression falling back to the check name with no gated job, both findings",
215+
PR_HEAD.replace("on: pull_request\n", "on:\n pull_request:\n workflow_dispatch:\n")
216+
+ PR_CHECK.replace(
217+
PR_CHECK_NAME,
218+
"${{ github.event_name == 'pull_request' && 'Other job' || '" + PR_CHECK_NAME + "' }}",
219+
),
220+
PR_CONTRACT,
221+
2,
222+
),
223+
(
224+
"an expression falling back to the check name beside one naming another job on every event",
225+
PR_HEAD.replace("on: pull_request\n", "on:\n pull_request:\n workflow_dispatch:\n")
226+
+ PR_CHECK.replace(
227+
PR_CHECK_NAME,
228+
"${{ github.event_name == 'pull_request' && 'Other job' || '" + PR_CHECK_NAME + "' }}",
229+
)
230+
+ PR_CHECK.replace("check-workflow-status:", "extra:").replace(
231+
PR_CHECK_NAME,
232+
"${{ github.event_name == 'pull_request' && 'Other job' || 'Other job' }}",
233+
),
234+
PR_CONTRACT,
235+
2,
236+
),
237+
(
238+
"an expression yielding the check name on every event, on a workflow a dispatch also triggers",
239+
PR_HEAD.replace("on: pull_request\n", "on:\n pull_request:\n workflow_dispatch:\n")
240+
+ PR_CHECK.replace(
241+
PR_CHECK_NAME,
242+
"${{ github.event_name == 'pull_request' && '"
243+
+ PR_CHECK_NAME
244+
+ "' || '"
245+
+ PR_CHECK_NAME
246+
+ "' }}",
247+
),
248+
PR_CONTRACT,
249+
1,
250+
),
186251
(
187252
"an expression yielding the check name on every event",
188253
PR_HEAD
@@ -633,6 +698,29 @@ def test_finding_counts(self) -> None:
633698
with self.subTest(label):
634699
self.assertEqual(len(audit.check_interface("wf", contract, text)), want)
635700

701+
def test_the_off_pull_request_finding_names_the_job_beside_a_gated_one(self) -> None:
702+
text = (
703+
PR_HEAD.replace("on: pull_request\n", "on:\n pull_request:\n workflow_dispatch:\n")
704+
+ PR_CHECK.replace(PR_CHECK_NAME, PR_ONLY_EXPRESSION)
705+
+ PR_CHECK.replace("check-workflow-status:", "extra:")
706+
)
707+
[(kind, message)] = audit.check_interface("wf", PR_CONTRACT, text)
708+
self.assertEqual(kind, "DRIFT")
709+
self.assertIn("job 'extra'", message)
710+
self.assertNotIn("check-workflow-status", message)
711+
712+
def test_an_every_event_expression_on_a_dispatch_workflow_is_its_job_finding(self) -> None:
713+
both = "' || '" + PR_CHECK_NAME + "' }}"
714+
text = PR_HEAD.replace(
715+
"on: pull_request\n", "on:\n pull_request:\n workflow_dispatch:\n"
716+
) + PR_CHECK.replace(
717+
PR_CHECK_NAME, PR_ONLY_EXPRESSION.replace("' || 'Check workflow status job' }}", both)
718+
)
719+
[(kind, message)] = audit.check_interface("wf", PR_CONTRACT, text)
720+
self.assertEqual(kind, "DRIFT")
721+
self.assertIn("job 'check-workflow-status'", message)
722+
self.assertIn("on a run other than pull_request", message)
723+
636724

637725
class ShippedPullRequestStubCase(unittest.TestCase):
638726
"""Every pull request stub the hub ships passes the contract spec/files.json gives a carried one."""

0 commit comments

Comments
 (0)