chore: fix CI workflow, restructure SKILL.md headers, sync README/router with disk#52
chore: fix CI workflow, restructure SKILL.md headers, sync README/router with disk#52josep-reyero wants to merge 2 commits into
Conversation
…ter with disk Phase 0 (bug fixes): - Fix license-header-check workflow placeholder SHA (@874b1c... -> @main). GitHub Actions could not resolve the placeholder; the workflow had never produced a single check run. Match the org-canonical pattern (~22/30 LF repos pin to @main, including the lfx-backstage scaffold template). - Add `with: copyright_line + include_files: "*.md"` to extend scanning to markdown (not in default filetypes). - Restructure all 15 SKILL.md license headers as YAML comments inside the frontmatter (line 2-3, after opening `---`). Required because the upstream workflow runs `head -4 | grep` and the previous HTML comments landed at line ~12, after frontmatter. YAML comments are silently ignored by the frontmatter parser; verified all 15 files still parse correctly with `name`, `description`, `allowed-tools` keys intact. - Add license header to README.md (top of file). - Fix wrong clone URL in 3 places: linuxfoundation/skills.git was working via GitHub redirect but is fragile; updated to linuxfoundation/lfx-skills.git in README.md (lines 8, 41) and docs/platform-install.md (line 15). Phase 1 (README/router coverage): - Update README's three skill lists (Verify autocomplete, Skill Overview table, Project Structure tree) to include all 15 skills. Previously missing: /lfx-pr-resolve, /lfx-git-setup, /lfx-snowflake-access, plus partial coverage of /lfx-cdp-snowflake-connectors. Also added the two undocumented coordinator references (fga-protected-types.md, indexed-data-types.md) and pr-resolve's evals/ directory to the tree. - Update /lfx routing table (lfx/SKILL.md) to cover the 5 unrouted skills: /lfx-pr-catchup, /lfx-git-setup, /lfx-intercom, /lfx-snowflake-access, /lfx-cdp-snowflake-connectors. Added matching Routing sections with Skill() invocation examples. /lfx-backend-builder and /lfx-ui-builder remain intentionally unrouted (invoked via /lfx-coordinator, not directly). - Add "Defer to specialized skills" section to /lfx-coordinator so it hands off Intercom and CDP-Snowflake-connector requests instead of attempting full-stack coordination on them. - Cross-link /lfx-pr-catchup → /lfx-pr-resolve in the drill-down step: if a drilled PR has unresolved comments, suggest invoking pr-resolve. - Document MCP prerequisites at the top of lfx-cdp-snowflake-connectors (lists the 5 mcp__claude_ai_LFX_BI_Layer__* tools the skill calls). Verification: - All 42 tracked .md files now have copyright in `head -4` (simulated the upstream workflow locally — it would PASS). - All 15 SKILL.md frontmatter still parses correctly via Ruby's YAML. - README's 3 lists exactly match `ls -d lfx*/`. - /lfx routing covers 12 user-facing skills (excludes builders by design). Skipped from the planned scope: - Standardizing skill descriptions with trigger phrases (deferred per reviewer direction — keep current descriptions). - .gitignore cleanup items (deferred). - install.sh "Available skills" output bug (will be made obsolete by the Phase 3 CLI rewrite). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
There was a problem hiding this comment.
Pull request overview
Restores and extends CI license-header enforcement while synchronizing the documented skill inventory across README and the /lfx router so docs, routing intent, and on-disk skills align.
Changes:
- Fixes the broken CI reusable-workflow reference and expands license-header scanning to Markdown files.
- Restructures SKILL.md frontmatter headers and updates
/lfxrouting intent + routing examples for previously unrouted skills. - Syncs README and install docs with the actual repo name/clone URL and current skill set on disk.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
README.md |
Adds license header, fixes clone URL, and updates skill lists/project structure to match on-disk skills. |
docs/platform-install.md |
Updates clone URL/directory name to the correct repo. |
.github/workflows/license-header-check.yml |
Restores the reusable CI workflow reference and configures it to include *.md files. |
lfx/SKILL.md |
Adds frontmatter license header and expands routing intent + routing sections for additional skills. |
lfx-coordinator/SKILL.md |
Adds frontmatter license header and instructs coordinator to defer certain requests to specialized skills. |
lfx-pr-catchup/SKILL.md |
Adds frontmatter license header and cross-links drill-down output to /lfx-pr-resolve. |
lfx-pr-resolve/SKILL.md |
Adds frontmatter license header. |
lfx-git-setup/SKILL.md |
Adds frontmatter license header. |
lfx-intercom/SKILL.md |
Adds frontmatter license header. |
lfx-snowflake-access/SKILL.md |
Adds frontmatter license header. |
lfx-cdp-snowflake-connectors/SKILL.md |
Adds frontmatter license header and documents MCP prerequisites/tools. |
lfx-preflight/SKILL.md |
Adds frontmatter license header. |
lfx-research/SKILL.md |
Adds frontmatter license header. |
lfx-product-architect/SKILL.md |
Adds frontmatter license header. |
lfx-setup/SKILL.md |
Adds frontmatter license header. |
lfx-test-journey/SKILL.md |
Adds frontmatter license header. |
lfx-ui-builder/SKILL.md |
Adds frontmatter license header. |
lfx-backend-builder/SKILL.md |
Adds frontmatter license header. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two of the three Copilot comments on PR #52: 1. Remove duplicate license header (the YAML frontmatter `# Copyright` from this branch + the pre-existing `<!-- Copyright -->` HTML comment after the closing `---` were both kept). Removed the redundant HTML `<!-- Copyright -->` and `<!-- SPDX-License-Identifier -->` lines across all 14 affected SKILL.md files. Kept the `<!-- Tool names ... -->` comment where present (different content, still useful as cross-platform guidance to readers). Collapsed the leftover blank lines so the post-frontmatter section stays clean. 2. Replace ambiguous `[number]` placeholder in lfx-pr-catchup's drill-down suggestion with `<pr-number>` and a concrete `#142` example to make it clear users shouldn't paste it verbatim. The third Copilot comment (SHA-pin the reusable workflow) is being addressed in a reply on the PR thread — sticking with @main per the org-canonical pattern (~22/30 LF repos including the lfx-backstage scaffold template pin to @main; supply chain risk is bounded since both repos are linuxfoundation-controlled). Verification: - All 15 SKILL.md still parse as YAML with required keys present. - All 15 still have copyright in `head -4` (license-header check passes). - No remaining `<!-- Copyright -->` HTML comments anywhere. - 13 files retain the tool-names comment (the 2 without it never had one). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
josep-reyero
force-pushed
the
chore/bug-fixes-and-coverage
branch
from
125aff2 to
65fc245
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -78,8 +78,6 @@ Every new `.ts`, `.html`, and `.scss` file MUST start with the appropriate licen | |||
|
|
|||
| **HTML (`.html`):** | |||
| ```html | |||
dealako
left a comment
dealako
left a comment
There was a problem hiding this comment.
Hey @josep-reyero 👋 — great housekeeping PR. The three-part scope (CI fix, license-header relocation, README/router sync) is well-reasoned and the execution is thorough: all 39 tracked .md files carry the header, the router routing sections match the intent table with no orphans, and the clone-URL fixes are complete. One over-deletion introduced a correctness bug that blocks merge, and the branch has picked up conflicts with main since PR #58 landed — both are straightforward to fix.
Step 1 — Issues
🔴 [blocking] lfx-ui-builder/SKILL.md:80 — Empty HTML license-header template
The cleanup that removed redundant <!-- Copyright --> / <!-- SPDX --> HTML comments caught one it shouldn't have: the .html example code block that tells the AI skill what header to write on every new Angular template. The block now reads:
**HTML (`.html`):**
```html
```
This is a correctness bug: the skill rule one line above says "Every new .ts, .html, and .scss file MUST start with the appropriate license header", but the example now instructs the AI to write nothing. Every Angular template generated by this skill will fail the license check that this very PR is wiring up.
Fix: restore the two header lines inside the block:
<!-- Copyright The Linux Foundation and each contributor to LFX. -->
<!-- SPDX-License-Identifier: MIT -->The TS and SCSS examples are unaffected because they use // comments, not <!-- -->.
🔴 [blocking] Merge conflicts — rebase required
mergeable: CONFLICTING. After PR #58 merged to main, there are content conflicts in:
README.mdlfx-intercom/SKILL.mdlfx/SKILL.md
A git rebase main (or merge) is needed before this can land.
🟡 [minor] .github/workflows/license-header-check.yml:10 — @main floating ref (already acknowledged)
Covered in depth in the existing thread — I agree with your reasoning. Same-org trust boundary + org-wide @main convention + this replaces a definitively broken placeholder = defensible tradeoff. Pinning a SHA + adding a Renovate entry would be the supply-chain-hardened next step if the team ever adopts a coordinated SHA-pinning policy, but it isn't a blocker here.
⚪ [nit] YAML frontmatter # comments — quick load-test before merge
The # Copyright / # SPDX lines sitting inside YAML frontmatter (before name:) are valid YAML, and you've empirically verified them in Claude Code. Just noting: a quick cross-platform check (e.g. a Gemini or Copilot parse, or another Claude Code user loading /lfx from a fresh install) before merge is cheap insurance if anyone in the org runs skills on a different platform.
Step 2 — AI Comment Reconciliation (Copilot)
| Copilot comment | Position | My assessment |
|---|---|---|
Duplicate YAML+HTML header (lfx/SKILL.md:3) |
✅ Resolved in 65fc245 | Agree — cleaned up correctly across all 14 affected files |
pr-catchup #[number] literal → template |
✅ Resolved in 65fc245 | Agree — <pr-number> + #142 example is clear |
@main supply-chain risk |
🔄 WONTFIX w/ justification | Agree with author's analysis — not blocking for same-org use |
Empty HTML block (lfx-ui-builder/SKILL.md:80) |
❌ Still unaddressed | Confirmed blocking — elevated above |
Step 3 — Revision Tracking
Resolved since last round (commit 65fc245):
- ✅ Duplicate header blocks removed across all 14 HTML-comment-carrying SKILL.md files
- ✅
lfx-pr-catchupdrill-down suggestion uses<pr-number>placeholder +#142example
Still unaddressed:
- ❌ Empty HTML block in
lfx-ui-builder/SKILL.md(Copilot, line 80, this round) - ❌ Merge conflicts (introduced by PR #58 landing on main after this PR was opened)
Summary
🔴 Blocking: 2 issues — empty HTML license template (lfx-ui-builder/SKILL.md), merge conflicts (README.md, lfx-intercom/SKILL.md, lfx/SKILL.md)
🟡 Minor: 1 issue — @main floating ref (already acknowledged/defended; not blocking)
⚪ Nit: 1 issue — frontmatter # comment cross-platform load-test suggestion
🔴 Needs changes before approval
The fix for the HTML block is a two-line restore; the rebase is mechanical. Once those land, the rest of the PR is in good shape to merge.
🤖 Review generated with Claude Code (Opus 4.8) — consolidated from independent analysis + security-auditor + code-reviewer-pro subagents.
| @@ -78,8 +78,6 @@ Every new `.ts`, `.html`, and `.scss` file MUST start with the appropriate licen | |||
|
|
|||
| **HTML (`.html`):** | |||
| ```html | |||
There was a problem hiding this comment.
[blocking] The over-deletion caught the .html example block, not just the document-level header. The block is now empty:
```html
This means the skill will instruct the AI to write **no header** on new Angular template files, which will fail the license-header CI check this PR is wiring up. The TS and SCSS blocks are fine (they use `//` comments); only the HTML block used `<!-- -->` and got caught by the cleanup.
**Restore these two lines inside the ```html block:**
```html
<!-- Copyright The Linux Foundation and each contributor to LFX. -->
<!-- SPDX-License-Identifier: MIT -->
|
@josep-reyero - do we still need this PR? |
Summary
This PR delivers two classes of change: structural fixes that restore broken CI and header enforcement, plus content sync so README, the
/lfxrouter, and the filesystem all agree on which skills exist.Bug fixes
CI workflow restored (
.github/workflows/license-header-check.yml)@874b1c3f4e5d6789abcdeffedcba1234567890abwith@main— the placeholder couldn't be resolved by GitHub Actions, so the workflow had never produced a single check run. ~22 of 30 LF repos pin to@main(matches the canonicallfx-backstagescaffold template).with:block to extend scanning to markdown viainclude_files: ""*.md""and pin the LFX-canonicalcopyright_line. JSON files (.markdownlint.json,evals.json) are intentionally excluded since JSON has no comment syntax.SKILL.md license headers restructured (all 15 files)
---(where they landed at line ~12, pasthead -4) into YAML#comments on lines 2–3 of the frontmatter (after the opening---). This keeps---on line 1 — required by the frontmatter parser — while putting the header within the workflow'shead -4 | grepwindow.name,description,allowed-toolskeys intact.<!-- Copyright -->/<!-- SPDX -->HTML comments after the closing---were removed across all 14 affected files. The<!-- Tool names ... -->cross-platform guidance comment is preserved where present.Other fixes
README.md.linuxfoundation/skills.git→linuxfoundation/lfx-skills.git(README.mdlines 8, 41;docs/platform-install.mdline 15). The old URL worked via GitHub redirect but breaks if anyone ever creates a reallinuxfoundation/skillsrepo.README/router/filesystem coverage
README synced with disk (3 lists, all now match
ls -d lfx*/)/lfx-pr-resolve,/lfx-git-setup,/lfx-snowflake-access, plus partial coverage of/lfx-cdp-snowflake-connectors.fga-protected-types.md,indexed-data-types.md) and pr-resolve'sevals/to the project structure./lfxrouter intent table extended (lfx/SKILL.md)/lfx-pr-catchup,/lfx-git-setup,/lfx-intercom,/lfx-snowflake-access,/lfx-cdp-snowflake-connectors. Now covers all 12 user-facing skills (/lfx-backend-builderand/lfx-ui-builderremain intentionally invoked via/lfx-coordinator, not directly).Other coverage fixes
/lfx-coordinatorlearned to defer Intercom and CDP-Snowflake-connector requests to the specialized skills./lfx-pr-catchupcross-links to/lfx-pr-resolvefrom the drill-down step when a PR has unresolved comments./lfx-cdp-snowflake-connectorsdocuments its MCP server prerequisite (5mcp__claude_ai_LFX_BI_Layer__*tools) so users know what to set up.Test plan
head -4 | grep ""Copyright The Linux Foundation""passes for all 42 tracked.mdfiles (simulated the upstream workflow locally — would PASS).ls -d lfx*/./lfxrouting table covers all 12 user-facing skills./lfx-git-setup) continue to load.mainshould be the first ever successful run for this repo — that's the proof.🤖 Generated with Claude Code