Skip to content

feat(scripts): add folder-consistency check and standardize WARN outp…#1350

Open
mariekekortsmit wants to merge 2 commits intomicrosoft:mainfrom
mariekekortsmit:feat/issue-1209-collection-folder-consistency
Open

feat(scripts): add folder-consistency check and standardize WARN outp…#1350
mariekekortsmit wants to merge 2 commits intomicrosoft:mainfrom
mariekekortsmit:feat/issue-1209-collection-folder-consistency

Conversation

@mariekekortsmit
Copy link
Copy Markdown

Description

Add a collection-id to folder name consistency check in Validate-Collections.ps1 that warns when a manifest item's folder doesn't match the collection id. Exempts shared/, hve-core/ folders and the hve-core-all collection. Also standardizes all Write-Warning calls to the Write-Host WARN pattern used throughout the script and normalizes WARN prefix spacing.

  • Add collection-id to folder name consistency check with exemptions for shared, hve-core, and hve-core-all
  • Replace Write-Warning calls with Write-Host WARN pattern for consistent output
  • Normalize WARN prefix spacing across all advisory messages
  • Add folder-consistency Pester tests with positive and negative Write-Host mock assertions

Related Issue(s)

Closes #1209

Type of Change

Select all that apply:

Code & Documentation:

  • Bug fix (non-breaking change fixing an issue)
  • New feature (non-breaking change adding functionality)
  • Breaking change (fix or feature causing existing functionality to change)
  • Documentation update

Infrastructure & Configuration:

  • GitHub Actions workflow
  • Linting configuration (markdown, PowerShell, etc.)
  • Security configuration
  • DevContainer configuration
  • Dependency update

AI Artifacts:

  • Reviewed contribution with prompt-builder agent and addressed all feedback
  • Copilot instructions (.github/instructions/*.instructions.md)
  • Copilot prompt (.github/prompts/*.prompt.md)
  • Copilot agent (.github/agents/*.agent.md)
  • Copilot skill (.github/skills/*/SKILL.md)

Other:

  • Script/automation (.ps1, .sh, .py)
  • Other (please describe):

Testing

  • All 45 Pester tests pass: npm run test:ps -- -TestPath "scripts/tests/collections/Validate-Collections.Tests.ps1"
  • 6 new folder-consistency tests cover: matching folder (no WARN), mismatched folder (WARN emitted), hve-core/ exemption, shared/ exemption, hve-core-all skip, and duplicate WARN output assertion
  • Tests use Mock Write-Host {} with Should -Invoke / Should -Not -Invoke and ParameterFilter matching WARN collection anchor

Checklist

Required Checks

  • Documentation is updated (if applicable)
  • Files follow existing naming conventions
  • Changes are backwards compatible (if applicable)
  • Tests added for new functionality (if applicable)

Required Automated Checks

The following validation commands must pass before merging:

  • Markdown linting: npm run lint:md
  • Spell checking: npm run spell-check
  • Frontmatter validation: npm run lint:frontmatter
  • Skill structure validation: npm run validate:skills
  • Link validation: npm run lint:md-links
  • PowerShell analysis: npm run lint:ps
  • Plugin freshness: npm run plugin:generate
  • Docusaurus tests: npm run docs:test

Security Considerations

  • This PR does not contain any sensitive or NDA information
  • Any new dependencies have been reviewed for security issues
  • Security-related scripts follow the principle of least privilege

Additional Notes

  • The folder-consistency check is advisory only (WARN, not FAIL) — it does not block validation
  • Write-Warning was the outlier pattern; Write-Host WARN is the established convention in this script per maintainer direction

…ut in collection validation

- add collection-id to folder name consistency check with exemptions for shared, hve-core, and hve-core-all
- replace Write-Warning calls with Write-Host WARN pattern for consistent output
- normalize WARN prefix spacing across all advisory messages
- add folder-consistency Pester tests with positive and negative Write-Host mock assertions

✨ - Generated by Copilot
@mariekekortsmit mariekekortsmit requested a review from a team as a code owner April 13, 2026 14:51
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.65%. Comparing base (f7f43d3) to head (ef79d01).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1350   +/-   ##
=======================================
  Coverage   87.65%   87.65%           
=======================================
  Files          61       61           
  Lines        9323     9334   +11     
=======================================
+ Hits         8172     8182   +10     
- Misses       1151     1152    +1     
Flag Coverage Δ
pester 85.23% <100.00%> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
scripts/collections/Validate-Collections.ps1 92.98% <100.00%> (+0.25%) ⬆️

... and 60 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@katriendg katriendg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mariekekortsmit for your contribution!

As I reviewed the current collections and what we are trying to achieve, we want to allow for some of the intentional cross-collection bundling. We want to catch folder names not matching collection names, but allow cross-bundling.

Could you update the check against all known collection IDs derived from the *.collection.yml filenames in collections folder. This catches real stale folders (like the original code-review/coding-standards/ drift from #1208) while allowing intentional cross-collection bundling.

Suggested fix:

  • Build a $knownCollectionIds set from all manifest filenames before the per-file loop.
  • Change the condition from $folderName -ne $id to -not $knownCollectionIds.ContainsKey($folderName).
  • Update the warning message to: "folder does not match any known collection ID".

You will probably just end up with one warning about a skill for playwright which we are aware of.
Thanks!

@mariekekortsmit
Copy link
Copy Markdown
Author

Thanks @mariekekortsmit for your contribution!

As I reviewed the current collections and what we are trying to achieve, we want to allow for some of the intentional cross-collection bundling. We want to catch folder names not matching collection names, but allow cross-bundling.

Could you update the check against all known collection IDs derived from the *.collection.yml filenames in collections folder. This catches real stale folders (like the original code-review/coding-standards/ drift from #1208) while allowing intentional cross-collection bundling.

Suggested fix:

  • Build a $knownCollectionIds set from all manifest filenames before the per-file loop.
  • Change the condition from $folderName -ne $id to -not $knownCollectionIds.ContainsKey($folderName).
  • Update the warning message to: "folder does not match any known collection ID".

You will probably just end up with one warning about a skill for playwright which we are aware of. Thanks!

So that means the relation should be bidirectional? All items under a specific prefix folder (e.g. .github/skills/experimental) should be inside the related collection.yaml, AND the collection should contain only items from the associated folder? E.g. in the playwright case, it is a skill inside skills/experimental/vscode-playwright folder, but it does not show in the experimental.collection.yaml, and that should issue a warning.

I only understood a one way relation, that explains our difference.

@katriendg
Copy link
Copy Markdown
Contributor

@mariekekortsmit The warning with the playwright is correct because it is missing in a collection experimental.

The issue with the current approach is it will also warn for an item from project-planning which is included in security collection, which is intentional. So in future, if a new folder my-collection is created, and this item is not found in a matching my-collection.collection.yml, that's when we want to warn.

Maybe in future we will want to warn about cross-bundling, but the current version contains several which we accept and don't want to warn about.

Hope that makes sense?

@mariekekortsmit
Copy link
Copy Markdown
Author

mariekekortsmit commented Apr 15, 2026

@mariekekortsmit The warning with the playwright is correct because it is missing in a collection experimental.

The issue with the current approach is it will also warn for an item from project-planning which is included in security collection, which is intentional. So in future, if a new folder my-collection is created, and this item is not found in a matching my-collection.collection.yml, that's when we want to warn.

Maybe in future we will want to warn about cross-bundling, but the current version contains several which we accept and don't want to warn about.

Hope that makes sense?

Maybe I'm confused because of the word "cross-bundling" and what that means exactly so
So the only case you want warnings for is if a new folder my-collection is created (so that means a folder underneath .github/agents/, .github/prompts/, .github/instructions/ and/or .github/skills/), and this item is not found in a matching my-collection.collection.yml, that's when we want to warn.

And with cross-bundling you mean, there is no check needed to verify that every item named in my-collection.collection.yaml indeed also appears in a folder that looks like .github/*/my-collection.

In the issue, one of the acceptance criteria is:
"The hve-core-all collection is exempt (it intentionally bundles items from all collections)"
But my current understanding based on your comments in this PR leads to all collections being exempt from that check, not only hve-core-all. Is that correct?

If my understanding still does not match the idea, maybe we can have a quick call to figure out where the misunderstanding is.

@katriendg
Copy link
Copy Markdown
Contributor

@mariekekortsmit The warning with the playwright is correct because it is missing in a collection experimental.
The issue with the current approach is it will also warn for an item from project-planning which is included in security collection, which is intentional. So in future, if a new folder my-collection is created, and this item is not found in a matching my-collection.collection.yml, that's when we want to warn.
Maybe in future we will want to warn about cross-bundling, but the current version contains several which we accept and don't want to warn about.
Hope that makes sense?

Maybe I'm confused because of the word "cross-bundling" and what that means exactly so So the only case you want warnings for is if a new folder my-collection is created (so that means a folder underneath .github/agents/, .github/prompts/, .github/instructions/ and/or .github/skills/), and this item is not found in a matching my-collection.collection.yml, that's when we want to warn.

And with cross-bundling you mean, there is no check needed to verify that every item named in my-collection.collection.yaml indeed also appears in a folder that looks like .github/*/my-collection.

In the issue, one of the acceptance criteria is: "The hve-core-all collection is exempt (it intentionally bundles items from all collections)" But my current understanding based on your comments in this PR leads to all collections being exempt from that check, not only hve-core-all. Is that correct?

If my understanding still does not match the idea, maybe we can have a quick call to figure out where the misunderstanding is.

I apologize for the confusion, the initial issue had the aim to also warn about cross-bundling. With cross-bundling we mean that an item from a collection project-planning is also referenced in security or vice-versa. Initially this was not the plan, which is what the issue describes as only hve-core and shared being allowed in cross-collection bundling.
But since we have others bundled, in the end I will update the acceptance criteria to match our current reality. My intention is for the future to fine tune this more towards our initial goal, but today we especially want to prevent a new folder/artifacts being created and not added to any collection. That's what we need to warn on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add collection-to-folder name consistency check to lint:collections-metadata

4 participants