fix(ci): make the license header check run, and cover markdown (LFXV2-2969) by josep-reyero · Pull Request #66 · linuxfoundation/lfx-skills

josep-reyero · 2026-08-03T16:53:28Z

Closes LFXV2-2969. Replaces #52, closed as obsolete — the diagnosis in it was right, the branch was not (it predates the skills/ layout and edits six skills that no longer exist).

The bug

.github/workflows/license-header-check.yml was pinned to @874b1c3f4e5d6789abcdeffedcba1234567890ab. That is not a commit — the API returns 422 No commit found for SHA, and the string is a hand-typed placeholder (...abcdef then 1234567890) that was never swapped for a real one.

Actions cannot resolve the ref, so it never builds a job. Every run since the file was added reports failure at 0s with total_count: 0 jobs and no logs. The check has never looked at a single file, while showing red on every PR.

What this does

1. Pins a real SHA (8313b4b, upstream main). Kept as a SHA rather than @main to preserve the original pin-don't-track intent; Dependabot can bump it. actions/checkout is pinned the same way (11d5960a, v4.4.0).

2. Splits the work by file type, not by header position. The shared workflow keeps every non-markdown file, where its four-line grep window is the right window. All markdown goes to .github/scripts/check-markdown-headers.sh.

Markdown needs its own checker for two reasons. The shared check greps the first four lines, and in a file with frontmatter --- must be line 1 with the header below the closing --- — outside that window no matter how short the frontmatter. It also only greps the copyright line, never SPDX, so it cannot enforce the full two-line header this repo uses.

3. Drops pull-requests: write. The reusable workflow declares contents: read and never writes, so nothing used it. The markdown job pins contents: read and checks out with persist-credentials: false.

The checker

One rule for all 67 markdown files: find where the header should start — below the closing --- when the file has frontmatter, line 1 when it does not — then require both the copyright and SPDX lines within four lines of it.

A leading --- is ambiguous, since it opens both frontmatter and a horizontal rule. They are told apart on content: a real frontmatter block carries a YAML key somewhere inside it, a rule is followed by prose. Not on a line budget — the deepest closing --- in the repo is already at line 20, so any fixed window would sit on top of files that exist.

Verification

.github/scripts/test-check-markdown-headers.sh runs 11 fixtures covering the three classification branches and the header window, each asserting on the message rather than only the exit code. It runs in CI ahead of the check itself.


Markdown	67 checked, pass (32 frontmatter, 35 plain)
Non-markdown	7 checked by the shared job, pass
Fixtures	11 passed, 0 failed

Cases covered: header on top; missing header; copyright without SPDX; header below frontmatter; header inside frontmatter (rejected); header pushed past the window; unterminated frontmatter; leading rule with and without a later rule.

No existing file content is moved or rewritten.

Review rounds

Copilot — frontmatter/no-header gap between the two jobs, stray pull-requests: write, SPDX enforced inconsistently across markdown. All three fixed in d56a788, which is where the two-job-by-path design was replaced by the one-checker-owns-markdown design above.
@dealako — unpinned actions/checkout, horizontal rule misclassified as frontmatter, no fixtures for the new branches, set -e. All four in 09d9263. The rule fix judges on block content rather than the proposed ~20-line window, for the reason given above; writing the fixtures then caught a second case where frontmatter opening with YAML comments would have had a header inside it accepted.

Follow-up (not blocking)

The root bug is upstream: head -4 is frontmatter-blind, which every LF repo with skills or agent definitions will hit. If lfx-public-workflows learns to skip a leading --- block, this repo can hand markdown back to the shared job.

🤖 Generated with Claude Code

…-2969) The workflow was pinned to @874b1c3f4e5d6789abcdeffedcba1234567890ab, which is not a commit. Read it and the intent is plain: abcdef followed by 1234567890 is a placeholder someone meant to swap and never did. The API returns 422 for it, so Actions could never resolve the reusable workflow and never built a job — every run since the file was added reports failure at 0s with zero jobs and no logs. A red check that has never once looked at a file. Pinned to a real upstream sha instead. That alone turns it green, but green over six files. Markdown was never in the shared workflow's default filetypes, so include_files brings in the 35 files without frontmatter, and copyright_line tightens the match from the loose upstream default to the full LFX line. The remaining 32 are the skills and the agents, and no input reaches them. The shared check greps the first four lines; frontmatter puts --- on line 1 and 3dc97ca settled that the header follows the closing ---, which lands it around line 10. Those two facts cannot both hold in a four line window, so the answer is not to move 32 headers back into frontmatter — 3dc97ca moved them out on purpose and that convention stands. They are excluded from the shared job and checked by a second one that looks where the convention actually puts the header. The two jobs partition the tree: 35 files without frontmatter to the first, 32 with it to the second, 67 tracked markdown files, no overlap and no gap. All 32 already comply, so this adds enforcement rather than fixing anything. Verified by replaying the upstream script's own exclude-pattern builder and scan loop locally, and by stripping a header, dropping an SPDX line, and adding a bare skill — each one fails the new job. Worth doing upstream eventually: teach the shared workflow to skip a leading frontmatter block before taking its window, and this repo drops both the exclusion and the second job. Diagnosis originally from PR #52, now closed as obsolete. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>

claude

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

Copilot

Pull request overview

Restores license-header CI and extends enforcement to Markdown files with YAML frontmatter.

Changes:

Pins the shared workflow to a valid commit and includes Markdown.
Adds a dedicated frontmatter-aware header-checking script and job.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`.github/workflows/license-header-check.yml`	Configures shared and frontmatter-specific checks.
`.github/scripts/check-frontmatter-headers.sh`	Validates headers following YAML frontmatter.

Review found a hole in the partition and it was real. The shared job excludes SKILL.md and agents/ by path; this script skipped files by content, on a missing opening ---. A skill or agent file with no frontmatter fell between the two and neither job looked at it, which is exactly the gap the PR claimed could not exist. The same split caused a second problem: the shared check only greps the copyright line, so the 35 markdown files it scanned were never checked for SPDX while the 32 here were. Two rules for one file type, described as one. Both come from splitting markdown across two checks, so the fix is to stop splitting it. The script now takes every tracked markdown file and picks where the header should start — below the closing --- when there is frontmatter, line 1 when there is not — then requires both lines within four of it. One rule, 67 files, no path exclusions to drift out of sync with the script. The shared job goes back to non-markdown, where its four line window is the right window and where the loose copyright-only match is all upstream offers. Renamed to check-markdown-headers.sh, since it is no longer only about frontmatter. Also dropped pull-requests: write. The reusable workflow declares contents: read and never writes, so nothing used it, and it was reaching the job that runs a script out of the PR checkout. That job now pins contents: read and checks out with persist-credentials: false — it only reads the tree. Verified: 67 markdown and 7 non-markdown files pass. A SKILL.md with no frontmatter, an agents/*.md with no frontmatter, a missing SPDX line on a non-frontmatter file, a stripped header below frontmatter, and unterminated frontmatter each fail. Reported by Copilot on PR #66. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

.github/workflows/license-header-check.yml:21

The PR description still documents the superseded 35/32 Markdown partition, include_files: "*.md", path exclusions, and a frontmatter-only second job. The current implementation instead sends all Markdown files through this job and leaves the reusable job to non-Markdown inputs. Update the description and verification counts so the documented coverage model matches the code being reviewed.

  # Every markdown file, frontmatter or not.
  markdown-header-check:

.github/workflows/license-header-check.yml:28

This newly added action is referenced through the mutable v4 tag, which defeats the SHA-pinning approach used for the reusable workflow and allows later tag movement to change code executed in this PR workflow. Pin the current v4 release to its full commit SHA instead.

        uses: actions/checkout@v4

dealako

Hey @josep-reyero — nice catch tracking down the placeholder SHA, and good instinct consolidating the frontmatter/non-frontmatter split into one script in d56a788 after Copilot flagged the partition gap. The core fix (real SHA pin, markdown coverage, least-privilege permissions) is solid and I verified the script passes cleanly against all 67 tracked markdown files as-is.

Revision tracking: all three of Copilot's first-round findings (frontmatter/no-header gap, stray pull-requests: write, inconsistent SPDX enforcement) were resolved in d56a788 — confirmed by re-reading the current script and workflow. Two of Copilot's later suppressed comments (stale PR description, unpinned actions/checkout@v4) are still valid and independently surfaced by this review below — I agree with both.

Findings:

🔴 Blocking: 0
🟡 Minor: 3 — unpinned actions/checkout@v4 inconsistent with this file's own pin-don't-track posture; a markdown file starting with a literal --- horizontal rule (not YAML frontmatter) would be misclassified by the frontmatter-detection branch; no automated test fixtures for the new script's branches
⚪ Nit: 1 — set -uo pipefail omits -e, so a failing awk/sed read would silently fall through rather than surface
❔ Question: 0

Also worth a look (not inline, no line to anchor it to): the PR description still documents the superseded 35/32 split design (include_files: "*.md", exclude_pattern, frontmatter-header-check job/script names, stale verification counts) from before d56a788's redesign — worth updating so the write-up matches what actually shipped.

Decision: 🔴 Needs changes before approval — nothing here blocks the CI fix from working, but the checkout pin and the frontmatter-detection edge case are worth a follow-up before merge, and there are 3 minor items total.

Review raised four things. Three are in; the fourth is in, but not the way it was proposed. A markdown file opening with a horizontal rule was read as frontmatter, and the suggested fix was to treat a leading --- as frontmatter only if it closes within about twenty lines. That window is already occupied: the deepest closing --- in the repo sits at line 20 today, so the next skill with a slightly longer description would be silently reclassified and then fail for a reason that has nothing to do with its header. Judging on content instead — a real frontmatter block carries a key, a rule is followed by prose — settles the same ambiguity without a budget to outgrow. Writing the fixtures then found a hole in that idea. Deciding on the single line below the delimiter reads frontmatter that opens with YAML comments as a rule, and then accepts a header sitting inside the frontmatter, which is the placement 3dc97ca deliberately moved out. So the key is looked for anywhere in the block. The test that caught it is in the suite. The suite is the second review point, and it was well aimed: this is the second edge case in two rounds that manual reasoning alone missed. Eleven fixtures over the three classification branches and the header window, each asserting on the message rather than only the exit code, because a check that fails for the wrong reason is its own bug. Wired in ahead of the check itself. actions/checkout is pinned to a sha, since a mutable tag sat oddly next to the deliberate pin two jobs above it. Pinned at v4.4.0 rather than the v4.2.2 in the suggestion, which is a couple of releases behind. set -e added. Guarded contexts are unaffected, and the suite passes unchanged. All 67 markdown files still classify as before: 32 frontmatter, 35 plain. Reported by dealako on PR #66. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>

josep-reyero · 2026-08-03T18:51:45Z

All four addressed in 09d9263, and the PR description is rewritten — it was still describing the two-job include_files/exclude_pattern design that d56a788 replaced, which was a fair catch.

Pinned actions/checkout to 11d5960a (v4.4.0) rather than the v4.2.2 in the suggestion — that SHA is correct for v4.2.2, but v4.4.0 is current.

The horizontal-rule case is real, but I did not take the proposed fix. A ~20-line window is already occupied: the deepest closing --- in this repo sits at line 20 today, so the next skill with a slightly longer description: would be silently reclassified and then fail for a reason unrelated to its header — the same misleading failure, moved somewhere harder to find. The disambiguation is on content instead: a real frontmatter block carries a YAML key, a rule is followed by prose. No budget to outgrow.

The fixtures earned their keep immediately. Writing them found a second case neither review had: judging on the single line below the delimiter reads frontmatter that opens with YAML comments as a rule, and then accepts a header sitting inside the frontmatter — the exact placement 3dc97ca moved out, and the format #52 wanted. So the key is looked for anywhere in the block. That case is now a test.

That is two edge cases in two rounds that manual reasoning missed and only surfaced when something systematic went looking, which is a better argument for the suite than anything I would have written in its defence.

11 fixtures, 67 markdown files, 7 non-markdown — all green.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

.github/scripts/check-markdown-headers.sh:51

The unterminated-frontmatter branch only inspects line 2, so it loses the comment-aware behavior used above. For example, an unclosed block with a comment on line 2, the two header strings on lines 3–4, and name: later is classified as plain and passes even though the header remains inside malformed frontmatter. Make the no-close classification handle YAML comments before the first key, and add this case to the fixtures.

  elif sed -n '2p' "$file" | grep -qE "$YAML_KEY"; then

.github/scripts/check-markdown-headers.sh:23

These values omit the required Markdown comment delimiters, so ordinary visible lines containing these strings satisfy the check even though they are not the repository's two MIT license comment lines. Match the complete HTML comment text; a malformed/plain-text pseudo-header should fail.

This issue also appears on line 51 of the same file.

COPYRIGHT="Copyright The Linux Foundation and each contributor to LFX."
SPDX="SPDX-License-Identifier: MIT"

dealako

Hey @josep-reyero — thanks for the fast turnaround. Three of the four items from the last round are genuinely well handled, and the fourth is close but has one real gap worth closing before merge.

👏 Nice work:

The classify() rewrite disambiguates a rule from real frontmatter by content (a YAML key anywhere in the block) rather than a line-position guess — and your own fixture-writing caught the YAML-comment-opened-frontmatter case neither of us had thought of. That's exactly the kind of edge case a systematic test suite is supposed to surface.
actions/checkout pinned to 11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 — good call going with current rather than the older SHA I suggested; either would have fixed the finding, but you picked the one that doesn't need a same-day Dependabot bump.
test-check-markdown-headers.sh is a solid harness: 11 fixtures, each a throwaway git repo (correctly modeling that the checker reads via git ls-files), asserting on the exact message rather than only the exit code.
You audited every guarded context before flipping on -e rather than just adding it and hoping — that's the right way to make that change.

Revision tracking:

✅ Resolved — actions/checkout@v4 unpinned tag → now SHA-pinned.
✅ Resolved — markdown horizontal rule misidentified as frontmatter → classify() now judges on block content, confirmed against the fixture suite.
✅ Resolved — PR description was stale → current body matches the shipped design (single script, 67/67 markdown, updated verification table).
⚠️ Partially addressed — set -uo pipefail missing -e → fixed in the main script, but the new test-check-markdown-headers.sh (line 12) still uses set -uo pipefail without -e.
⚠️ Partially addressed — no automated tests → 11 fixtures landed, but see the blocking finding below: the specific case that would have caught it isn't among them.

New findings this round:

🔴 Blocking: 1 — the unterminated branch in classify() only inspects line 2 for a YAML key, unlike the fm branch which scans the whole block. A file that opens ---, has a YAML comment on line 2, a real key on line 3+, and never closes gets misclassified as plain and can pass with its header sitting inside malformed frontmatter — the exact bug class this round's fix targeted, just relocated to the sibling branch. This is Copilot's suppressed comment on this PR, and I independently confirmed it's real and reachable.

🟡 Minor: 1 — COPYRIGHT/SPDX are matched via grep -qF against the raw text, without requiring the surrounding comment delimiter, so a plain-prose line containing that exact string (not inside a real  or # comment) would also satisfy the check. Pre-existing, not introduced by this commit, but Copilot's second suppressed comment raises it and I agree it's a real (if low-probability) gap worth a follow-up.

⚪ Nit: 1 — test-check-markdown-headers.sh:12 still has set -uo pipefail; bring it in line with the main script's now-audited set -euo pipefail.

Decision: 🔴 Needs changes before approval — the blocking item is the same failure mode as last round's headline fix, just on the branch's twin; closing it (and ideally adding a fixture for it) should be quick given the harness that's already in place.

…iters The blocking finding was right, and it is the same bug as last round sitting on the branch next to the one that was fixed. Where frontmatter never closes, the classification was decided on the single line below the delimiter, so a block opening with YAML comments read as a horizontal rule; the header inside that broken frontmatter then satisfied the check from the top of the file. Built the case before changing anything and it passed, which it should not have. It now scans the block the same way the closed branch does. Two rounds, two variants of one mistake: deciding a block's nature from one line of it. Both are fixtures now. The header is also matched with its comment delimiters rather than as bare text. All 67 markdown files already use the exact `` form, so nothing moved, but prose containing the sentence no longer counts, and a header left inside frontmatter as a YAML `#` comment no longer counts either — which closes the same hole from the other side. set -e added to the harness, though not as suggested. A bare out=$(...) aborts under -e the moment a case is meant to fail, and most cases here are meant to fail, so the status is captured through if instead. Verified the naive form does abort before writing the alternative. 13 fixtures, 67 markdown files, classification unchanged at 32 frontmatter and 35 plain. Reported by dealako on PR #66. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

.github/scripts/check-markdown-headers.sh:48

YAML_KEY does not distinguish frontmatter from prose. A plain document beginning with ---, followed by the required header and a normal line such as Note: text, is classified as frontmatter (or as unterminated when there is no later rule), so the checker rejects a correctly headed form that the horizontal-rule fixtures intend to support. Use an unambiguous repository rule for line-one ---, or a classifier that does not treat every colon-prefixed prose line as proof of frontmatter, and add this shape to the fixtures.

    if sed -n "2,$((close - 1))p" "$file" | grep -qE "$YAML_KEY"; then

.github/scripts/check-markdown-headers.sh:90

grep -qF accepts the marker as a substring, so inline prose containing the HTML marker—or even both markers concatenated onto one physical line—passes despite not providing the required license-header lines. Match complete lines so this checker actually enforces two comment lines.

  if ! grep -qF "$COPYRIGHT" <<<"$window"; then
    echo "$file is missing the license header (expected within 4 lines of line $start)"
    status=1
  elif ! grep -qF "$SPDX" <<<"$window"; then
    echo "$file is missing '$SPDX' (expected within 4 lines of line $start)"

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI balanced review requested due to automatic review settings August 3, 2026 16:53

claude Bot reviewed Aug 3, 2026

View reviewed changes

Copilot started reviewing on behalf of josep-reyero August 3, 2026 16:53 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Comment thread .github/scripts/check-frontmatter-headers.sh Outdated

Comment thread .github/workflows/license-header-check.yml

Comment thread .github/workflows/license-header-check.yml Outdated

Copilot AI review requested due to automatic review settings August 3, 2026 17:07

Copilot started reviewing on behalf of josep-reyero August 3, 2026 17:07 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

dealako requested changes Aug 3, 2026

View reviewed changes

Comment thread .github/workflows/license-header-check.yml Outdated

Comment thread .github/scripts/check-markdown-headers.sh Outdated

Comment thread .github/scripts/check-markdown-headers.sh Outdated

Comment thread .github/scripts/check-markdown-headers.sh

Copilot AI review requested due to automatic review settings August 3, 2026 18:48

Copilot started reviewing on behalf of josep-reyero August 3, 2026 18:48 View session

josep-reyero requested a review from dealako August 3, 2026 18:51

Copilot AI reviewed Aug 3, 2026

View reviewed changes

dealako requested changes Aug 3, 2026

View reviewed changes

Comment thread .github/scripts/check-markdown-headers.sh Outdated

Comment thread .github/scripts/test-check-markdown-headers.sh Outdated

Comment thread .github/scripts/check-markdown-headers.sh Outdated

Copilot AI review requested due to automatic review settings August 3, 2026 21:57

Copilot started reviewing on behalf of josep-reyero August 3, 2026 21:57 View session

josep-reyero requested a review from dealako August 3, 2026 21:57

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Merge branch 'main' into fix/LFXV2-2969-license-header-check

6f8c90c

Copilot AI review requested due to automatic review settings August 6, 2026 16:11

Copilot AI reviewed Aug 6, 2026

Uh oh!

Conversation

josep-reyero commented Aug 3, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The bug

What this does

The checker

Verification

Review rounds

Follow-up (not blocking)

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

dealako left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josep-reyero commented Aug 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

dealako left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

josep-reyero commented Aug 3, 2026 •

edited by atlassian Bot

Loading