Reject pull_request_target in CI, so a sync cannot reintroduce it - #229

Trigger the lint when its checker changes

When a PR or push modifies only scripts/unsloth/check_workflow_triggers.py, this workflow will not run because neither push.paths nor pull_request.paths includes that file (only test_*.py and the two older workflow checkers are listed). Consequently, a regression or weakening of this security check can merge without executing either the checker or its tests; add the new checker path to both trigger lists.

Useful? React with 👍 / 👎.

Correct, and it is the same defect class this change exists to close, applied to the security check itself. The lint's filters listed test_*.py and the two older checkers but not scripts/unsloth/check_workflow_triggers.py, so a commit weakening the checker would not have run it.

Added to both trigger lists in 4690af5. Note the test file was already covered by the scripts/unsloth/test_*.py glob, so the gap was the checker alone, which is the more dangerous half of the pair.

Require the workflow_run waiver to include a reason

When a workflow contains the marker by itself—or merely embeds it in a quoted run: string—this substring check treats the restricted workflow_run trigger as justified. That lets a workflow bypass the policy without the written safety argument promised by the diagnostic and module documentation; validate an actual comment with a nonempty accompanying justification instead of checking only for the marker anywhere in the file.

Useful? React with 👍 / 👎.

Right on both readings of the substring test: the marker counted with no argument at all, and it counted inside a quoted run: string. Either way a workflow could claim the exemption without recording the reason, which is the only thing requiring a marker buys.

4690af5 adds waiver_status, which requires the marker on a COMMENT line and a non-empty reason, either on that line or on the comment lines directly beneath it, since a justification usually runs onto the next line. Three cases added to the test: the marker alone fails, the marker inside a run: string fails, a reason on the line beneath passes. The two live workflow_run waivers already carry their reasons, so neither changed.

For the record, unslothai/unsloth's scripts/lint_workflow_triggers.py has the same substring behaviour. That one predates this work and I have left it alone rather than widen this change, but it is worth the same treatment.

Reject waiver markers inside YAML block scalars

Checking line.strip().startswith("#") still treats shell comments inside a multiline run: | scalar as YAML comments. A workflow_run workflow can therefore put the marker and a purported reason on consecutive comment lines inside its script and waiver_status() returns success even though the file has no actual waiver comment. Fresh evidence beyond the earlier quoted-string case is that the common block-scalar form remains accepted; identify comments using YAML token/source locations rather than stripped raw lines.

Useful? React with 👍 / 👎.

Correct, and the fresh-evidence framing is right: the round-1 fix closed the quoted single-line case and left the multi-line one, which is how run: is actually written. A block scalar body is opaque text, # opens a comment in shell exactly as in YAML, and a line-by-line reader cannot tell them apart.

_block_scalar_lines now computes the extent of every block scalar and those lines no longer count, for the marker or for the reason lines beneath it. A block scalar runs until the indentation returns to its key level, blank lines included, so the extent is computable without YAML token positions; I stayed with the line reader rather than moving to a parser because the rule is about where a comment sits in the source and the reader is what the sibling checks in this repo use.

Three checks added: the block-scalar marker fails and reports the marker as absent rather than unreasoned, and the same file with a real comment above it passes, so the rule rejects the location and not the justification. Pushed as 7762e2c.

Recognize both valid block-scalar indicator orders

When a workflow uses the valid YAML spelling run: |2- (indentation indicator before chomping indicator), this regex does not recognize the scalar opener, so a marker comment inside that script body is treated as a real waiver and the restricted workflow_run passes. Fresh evidence beyond the previously reported block-scalar case is that the new matcher accepts |-2 but not the equivalent |2-; handle both orders or derive scalar source ranges from the YAML parser.

Useful? React with 👍 / 👎.

Correct. YAML fixes no order between the chomping and the indentation indicator, so |2- is as valid as |-2, and my [|>][-+]?\d* matched only the second. The consequence is the one you name: the opener is not recognised, so the entire body of that step goes back into scope as ordinary comment lines, which is the hole the block-scalar fix had just closed reached by a different spelling of the same step.

Now [|>](?:\d+[-+]?|[-+]?\d*), which takes |, >, |-, |2, |-2 and |2-. Test added over the |2- form of the same fixture.

Require an explicit justification after a bare marker

When a bare marker is followed by any unrelated comment, such as the repository's standard # SPDX-License-Identifier: ... header, this branch accepts that text as the reason and waives workflow_run without a safety argument. Fresh evidence after the earlier bare-marker fix is that placing the marker immediately above the existing license header still passes; require an explicit continuation such as # Justified: ... rather than treating every adjacent comment as justification.

Useful? React with 👍 / 👎.

Agreed, and it is worse than a generic weakness here: every workflow in this repository opens with # SPDX-License-Identifier and # Copyright, so a bare marker placed directly above that header waived the trigger while saying nothing, and it would read as perfectly ordinary in a diff. The easiest reason in the tree to borrow by accident.

A continuation line must now announce itself with # Justified:, and Justified: with nothing after it is rejected too. This costs nothing on the live files, because both existing waivers already write # Justified: ... on the line under the marker, which is why the introducer is the right shape for the rule rather than a new burden. The failure message names the required form so the documented remedy is the one that works.

Three checks: the license header rejected, the empty Justified: rejected, and the real form still passing. Pushed as 7b268e7.

Recognize spaced block-scalar mappings

When a workflow uses valid YAML such as run : |, PyYAML still parses the value as a block scalar, but this regex does not match because it requires the colon immediately after the key. A marker in that script body is consequently omitted from in_block and accepted as a real waiver, allowing an unjustified workflow_run workflow to pass. Fresh evidence beyond the previously reported block-scalar indicator bypass is that run : | with # lint:workflow_triggers-allow-workflow_run fake in its body makes this checker return success; accept valid key/colon separation or derive scalar ranges from the parsed YAML.

Useful? React with 👍 / 👎.

x

Reject an empty inline Justified label

When the waiver is written as # lint:workflow_triggers-allow-workflow_run Justified:, stripping punctuation leaves Justified, so this branch accepts it even though no safety argument follows the label. Fresh evidence after the bare-marker fix is that this exact inline spelling still makes an otherwise unjustified workflow_run pass, while the equivalent separate bare # Justified: line is correctly rejected; handle the optional inline label explicitly and require text after it.

Useful? React with 👍 / 👎.

Exclude multiline quoted scalars from waiver comments

When a single- or double-quoted YAML scalar spans multiple lines and a continuation line begins with # lint:workflow_triggers-allow-workflow_run reason, PyYAML treats that line as scalar content, but _block_scalar_lines() records only | and > nodes. I verified that an active workflow_run containing this marker inside a multiline quoted run: value returns success. Fresh evidence beyond the earlier single-line quoted-string case is that valid multiline quoted scalars bypass the new startswith("#") protection; exclude every parser-identified quoted or block scalar source range.

Useful? React with 👍 / 👎.

Correct, and the general form is better than the enumeration I had. I checked it directly: a double-quoted run: spanning three lines reports style " with start (7,13) and end (9,16), and the # line in the middle is scalar content that a |-and->-only version read as a comment.

Every scalar style is now included, which needs no list at all: whatever lies strictly inside a scalars extent is that scalars value, by definition of the marks. Renamed to _scalar_content_lines, since it is no longer about block scalars specifically. test_the_marker_inside_a_multiline_quoted_scalar_does_not_count covers it. Pushed as b2c264c.

Stop including the comment after a block scalar

When a real waiver comment is placed immediately after a run: | block, PyYAML reports the scalar's end_mark.line as that following comment line with column zero. Including the endpoint therefore adds the waiver to in_block, causing this otherwise valid workflow_run file to fail with “has no ... comment”; I verified this exact layout returns exit code 1. Use the end column to distinguish content ending on a line from an exclusive line boundary instead of unconditionally including the endpoint.

Useful? React with 👍 / 👎.

Confirmed against PyYAML before fixing, and it is the worse of the two errors this rule can make: a false failure on a correct file. For a run: | block followed by a dedented line, the scalar reports end_mark as (that line, column 0), so including the endpoint unconditionally put the waiver comment itself into the excluded set and the file failed with "has no comment". The next person to write the waiver at the bottom of a file would have hit it.

The end is now exclusive when end_mark.column is 0. test_a_waiver_comment_right_after_a_run_block_still_counts uses exactly that layout: run: |, echo hi, then the waiver at column 0.

Require the exact waiver directive

When a comment contains a near-match such as # lint:workflow_triggers-allow-workflow_runs, this substring test recognizes the canonical marker and then treats the trailing s as a nonempty justification, so an unjustified workflow_run passes. Fresh evidence after the quoted/block-scalar fixes is that this exact spelling makes the checker exit successfully; require the comment directive to end at a token boundary before parsing its reason.

Useful? React with 👍 / 👎.

Correct, and I checked it before changing anything because the asymmetry is striking: # lint:workflow_triggers-allow-workflow_runs returned (True, "") while the correctly spelled bare marker on the same input was refused. A typo was a more effective waiver than the real directive.

Matched with a (?![\w-]) boundary now, so the directive has to end where it ends. Six checks: the s, -anyway and _now suffixes all refused, and the canonical form followed by a space, a colon or Justified: all still reading their reason. Pushed as the head commit.

Anchor the waiver directive at the comment start

Fresh evidence beyond the previously reported suffix near-match is that this unanchored search accepts the marker anywhere within a comment. For example, # Never use lint:workflow_triggers-allow-workflow_run without review makes a workflow_run file pass because without review is interpreted as its justification, even though the comment explicitly is not a waiver directive. Match the directive at the beginning of the YAML comment body before parsing its reason.

Useful? React with 👍 / 👎.

Correct, and it returned (True, "") on exactly your example before the fix. A comment saying the opposite of a waiver was read as one, with the rest of the sentence as its justification.

The directive now has to OPEN the comment as well as end at a token boundary, so the comment body must START with it. Three checks, one each for the Never use ..., TODO: drop the ... and see docs for ... shapes.

Run the trigger guard for large sync diffs

When an upstream sync or PR changes more than 300 files, GitHub evaluates paths filters using only the first 300 changed files; if the workflow that reintroduces pull_request_target is outside that subset and none of this workflow's other listed paths are included, unsloth-pr-set-lint.yml never starts and this new step cannot scan the change. That is precisely the large-sync scenario the guard is intended to cover, so host the security check in an unconditionally triggered workflow or otherwise ensure syncs cannot skip it.

Useful? React with 👍 / 👎.

Accepted, and this is the most useful finding on the PR, because it is the one that makes the check not apply in the scenario it was written for. The module docstring claims the host was chosen so that every workflow edit runs it; the 300-file paths evaluation limit makes that claim conditional exactly when a diff is large, and an upstream sync is routinely far larger than 300 files.

Fixed by giving the check a second home in unsloth-upstream-sync-guard.yml, which has no paths filter and already runs on every push to master, which is how a sync arrives. Keeping it in both places is deliberate: the lint host gives fast feedback on an ordinary pull request, the sync guard gives the guarantee. It is also the natural home, since that workflow already refuses to trust a truncated compare-API file list for the same underlying reason, and now says so in a comment beside the new step.

What this does not cover, stated plainly: a pull request over 300 files can still skip the lint host at PR time. The push-to-master run then catches it, which is the protection level the docstring describes. Pushed as b4ab517.

Run the sync check before merging

For a sync PR whose workflow file falls outside GitHub's 300-file path-filter window, this new second host still cannot block the unsafe workflow: unsloth-upstream-sync-guard.yml subscribes only to pushes to master, so this step first runs after the sync has merged and the reintroduced pull_request_target workflow is already active. The new push-only placement is fresh evidence beyond the earlier path-filter finding; add an unfiltered pull_request trigger or another required pre-merge check for syncs rather than relying on a post-merge failure.

Useful? React with 👍 / 👎.

Correct, and it is the right escalation of the previous finding rather than a repeat of it: I moved the check to a host with no paths filter but left it push-only, so for the exact case being discussed it ran after the merge, with the reintroduced workflow already active. That is a notification, not a guard.

The sync guard now also subscribes to pull_request, unfiltered, so the trigger check can fail a sync before it lands. The two existing steps ask whether a recorded upstream commit is still an ancestor of master and whether the fork delta stays inside .github/ and scripts/unsloth/; both are questions about master's own history and are meaningless against a pull request head, so they keep their current behaviour behind if: github.event_name != 'pull_request'. Only the trigger check runs on both events.

Reject punctuation-only waiver reasons

Fresh evidence after the earlier empty-label fix is that a comment such as # lint:workflow_triggers-allow-workflow_run !!! still passes: the punctuation survives tail.strip(" #:-"), so this branch returns success even though no written safety argument exists. This allows an unjustified workflow_run workflow to bypass the policy; require substantive text after any optional delimiter or label rather than merely a nonempty punctuation string.

Useful? React with 👍 / 👎.

Correct, confirmed before fixing: both !!! and *** returned (True, ""). The punctuation survived the strip and stood in for the argument the marker exists to record.

_is_reason now requires three word characters, applied to both the inline form and the # Justified: continuation line. The threshold is arbitrary in the way any threshold here would be, and the docstring says so along with the honest scope of the rule: it rules out the degenerate case only. Whether an argument is a GOOD argument is a question for the reviewer, not for a lint, and I would rather write that down than imply the check is doing more than it is.

Nine checks: four punctuation shapes each way, plus a short real reason (no checkout) still passing, so the threshold does not quietly start demanding an essay.

Reject underscore-only waiver reasons

Fresh evidence after the punctuation-only fix is that Python's \w includes underscores, so a waiver such as # lint:workflow_triggers-allow-workflow_run ___ satisfies this threshold and lets an unjustified workflow_run pass. Require actual alphanumeric prose (preferably at least one letter) rather than regex word characters, which treat _ as substantive text.

Useful? React with 👍 / 👎.

Correct, and a good catch on my own fix: Python's \w includes the underscore and digits, so ___ cleared the threshold while saying exactly as much as the !!! the threshold was added to reject.

Counted as letters now, [^\W\d_]. Three checks added: ___, 123 and _ _ _, each rejected inline and on a # Justified: continuation line, with the short real reason still passing so the threshold does not start demanding an essay. The docstring keeps the same framing and is a raw string now so the \w in it is not a syntax warning.