From 74b49d3eda2755a70b81bddf71dc9c248a38f2de Mon Sep 17 00:00:00 2001 From: Trent Blackburn Date: Mon, 24 Aug 2026 12:09:07 -0400 Subject: [PATCH 1/2] chore: Sync AI agent instructions to AIM 0.12.0 Four versions behind: 0.8.14, released 2026-05-16, against 0.12.0 from 2026-08-19. Ignoring line endings, only three instruction files actually differ; the rest are identical. powershell.instructions.md gains a Pester section carrying six findings measured during a Pester 5 to 6 migration. Three of them describe traps this repository has already hit or can still hit: -Skip: is evaluated at discovery so it cannot read a BeforeAll variable, an It-level -ForEach does not bind $_ for the skip condition, and gating a build on Invoke-Pester results needs FailedBlocksCount and FailedContainersCount rather than FailedCount alone. update.instructions.md rewrites the skill-dependency step for the 0.10.0 vendoring model and renumbers the procedure accordingly. git-workflow.instructions.md lowercases its ticket-identifier examples. repository-specific.instructions.md is never copied from upstream and was left alone. aim.config.json needs no change: the module list is unchanged, and the skills block stays absent, so the new vendoring step is inert. Line endings converted to CRLF to match the repository, so the diffs show content rather than every line. Synced per instructions/update.instructions.md, with per-file confirmation. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012AKaM9i6NyMFDcJNeC34h5 --- AGENTS.md | 20 +- instructions/git-workflow.instructions.md | 9 +- instructions/powershell.instructions.md | 359 +++++++++++++++++++++- instructions/update.instructions.md | 57 +++- 4 files changed, 428 insertions(+), 17 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 177aaf5..c3adfae 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,9 +2,9 @@ AI agents working in this repository must follow these instructions. -Template Version: 0.8.14 +Template Version: 0.12.0 -Last sync: 2026-05-17 (Update this date when syncing from the centralized repository) +Last sync: 2026-08-24 (Update this date when syncing from the centralized repository) ## Instructions for AI Agents @@ -16,7 +16,7 @@ AI agents **must**: 2. **Read `instructions/agent-workflow.instructions.md` FIRST to determine which other instruction files apply to your task.** Follow all applicable instructions before proceeding with work. -3. **Check `aim.config.json`** for module configuration and external source settings. +3. **Check `aim.config.json`** for module configuration, external source, and skill dependency settings. ## Instruction Applicability Matrix @@ -70,3 +70,17 @@ Use this matrix to determine which instruction files to read based on your task: ## Repository-Specific Instructions See `instructions/repository-specific.instructions.md` for customizations specific to this repository. + +## Skill Dependencies + +A repository can vendor Agent Skills (the open [Agent Skills](https://agentskills.io) `SKILL.md` +standard) it depends on, declared in `aim.config.json` under `skills`. Unlike a per-developer +install, the skills are checked in under `skills.vendorPath` (default `.agents/skills/`) - the +cross-client convention - so they travel with the repository and any agent can use them. When a +skill is vendored, a row is added to the Instruction Applicability Matrix above mapping its task +type to `//SKILL.md`, routing it alongside the instruction files; agents that +natively scan `.agents/skills/` also pick it up directly. Because Claude Code reads `CLAUDE.md` +rather than `AGENTS.md`, a `CLAUDE.md` that imports +this file (`@AGENTS.md`) carries the routing into Claude Code. When a skill covers a task (for +example build and test tooling), prefer its guidance over ad-hoc commands. See +`instructions/update.instructions.md` for how skills are vendored and routed during sync. diff --git a/instructions/git-workflow.instructions.md b/instructions/git-workflow.instructions.md index 9c3d96c..5f829cc 100644 --- a/instructions/git-workflow.instructions.md +++ b/instructions/git-workflow.instructions.md @@ -39,6 +39,9 @@ When using project management tools, include the ticket identifier: /- ``` +Lowercase the ticket identifier even when the tracker displays it in uppercase (`PROJ-123` +becomes `proj-123`), so the whole branch name stays lowercase. + ### Branch Types | Prefix | Purpose | Example | @@ -55,9 +58,9 @@ When using project management tools, include the ticket identifier: ### Examples with Ticket Numbers ```text -feature/PROJ-123-add-user-authentication -bugfix/PROJ-456-fix-login-validation -hotfix/PROJ-789-patch-security-issue +feature/proj-123-add-user-authentication +bugfix/proj-456-fix-login-validation +hotfix/proj-789-patch-security-issue ``` ### Best Practices diff --git a/instructions/powershell.instructions.md b/instructions/powershell.instructions.md index 8493861..12d477a 100644 --- a/instructions/powershell.instructions.md +++ b/instructions/powershell.instructions.md @@ -42,6 +42,13 @@ function Get-Data { # Good - separate functions at module/script scope function Format-Result { + <# + .SYNOPSIS + Formats a raw result object for display. + + .PARAMETER Value + The raw result object to format. + #> [CmdletBinding()] [OutputType([psobject])] param( @@ -55,6 +62,13 @@ function Format-Result { function Get-Data { + <# + .SYNOPSIS + Retrieves the data record for a named entity. + + .PARAMETER Name + The name of the entity to retrieve. + #> [CmdletBinding()] [OutputType([hashtable])] param( @@ -69,6 +83,13 @@ function Get-Data { # Function with pipeline input function Get-PipelineInput { + <# + .SYNOPSIS + Processes each item received from the pipeline. + + .PARAMETER InputData + The item to process, accepted from the pipeline. + #> [CmdletBinding()] [OutputType([PSCustomObject])] param( @@ -140,36 +161,54 @@ $users = Get-ADUser -Filter { Enabled -eq $true } Use the appropriate suffix to indicate what the variable holds: -- Use `Path` for any path string (file or folder) -- Reserve `Directory` for directory objects (e.g., `[System.IO.DirectoryInfo]`) or bare folder names +- Use `Path` for any string that names a location, whether it points at a file or a folder, + and whether it is absolute, relative, or a bare folder name +- Reserve `Directory` for directory objects (e.g., `[System.IO.DirectoryInfo]`) ```powershell # Good - Path suffix for path strings $configurationPath = Join-Path -Path $PSScriptRoot -ChildPath 'config.json' $outputPath = Join-Path -Path $PSScriptRoot -ChildPath 'results' $backupPath = 'C:\Backups' +$moduleFolderPath = 'MyModule' # Good - Directory suffix for a directory object $logDirectory = [System.IO.DirectoryInfo]::new('C:\Logs') # Bad - Directory suffix on a path string $outputDirectory = 'C:\App\results' +$moduleFolderDirectory = 'MyModule' ``` ## Parameters -1. Use full parameter names in scripts and functions +1. Name parameters on calls that pass two or more arguments; a single-argument call may stay + positional. Naming disambiguates which value maps to which parameter when there are several; + with one argument there is nothing to disambiguate, so naming it only adds noise. 2. Always use quotes around string parameter values 3. Include validation on every parameter 4. Place each component on its own line +```powershell +# Good - 2+ arguments: name them (no positional guessing) +Get-ChildItem -Path 'C:\Logs' -Filter '*.log' -Recurse +Copy-Item -Path $sourcePath -Destination $destinationPath + +# Good - single argument: positional is fine +Test-Path $configurationPath +Import-Module $modulePath + +# Avoid - naming the only argument adds noise without removing ambiguity +Test-Path -Path $configurationPath +``` + ```powershell # Good - string parameter values are quoted -Get-Process -Name 'powershell' +Get-Process 'powershell' Get-ChildItem -Path 'C:\Program Files' -Filter '*.txt' # Bad - bare string parameter values -Get-Process -Name powershell +Get-Process powershell Get-ChildItem -Path C:\Program Files -Filter *.txt ``` @@ -338,13 +377,13 @@ function Connect-Service { [Parameter()] [ValidateNotNull()] - [System.Management.Automation.PSCredential] + [PSCredential] [System.Management.Automation.Credential()] - $Credential = [System.Management.Automation.PSCredential]::Empty + $Credential = [PSCredential]::Empty ) # Check if credentials were provided - if ($Credential -eq [System.Management.Automation.PSCredential]::Empty) { + if ($Credential -eq [PSCredential]::Empty) { # Use current user context } else { @@ -513,3 +552,307 @@ Suppressions without justification are not acceptable: # Never do this [Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseSingularNouns', '')] ``` + +## Pester + +### Skipping Tests + +`Set-ItResult -Skipped` (and `-Inconclusive`) ends the `It` block immediately - it throws an +internal error record that Pester catches and records as the test result. Code after the call +does not run, so a trailing `return` is unreachable dead code; do not add one. Reviewers, +including automated ones, recurrently suggest the redundant `return`. + +```powershell +# Good - Set-ItResult ends the test; nothing after it runs +It 'Validates the required version' { + if (-not $dependency.ContainsKey('RequiredVersion')) { + Set-ItResult -Skipped -Because 'No RequiredVersion to validate' + } + Test-VersionConstraint -Version $dependency.RequiredVersion | Should -BeTrue +} + +# Bad - the return can never execute; Set-ItResult already threw +It 'Validates the required version' { + if (-not $dependency.ContainsKey('RequiredVersion')) { + Set-ItResult -Skipped -Because 'No RequiredVersion to validate' + return + } + Test-VersionConstraint -Version $dependency.RequiredVersion | Should -BeTrue +} +``` + +Prefer `-Skip:$condition` on `It`, `Context`, or `Describe` when the condition is known at +discovery time; reserve `Set-ItResult -Skipped` for conditions only known at runtime inside +the test body. + +```powershell +# Good - a discovery-time condition uses the -Skip parameter +It 'Runs only on Windows' -Skip:(-not $IsWindows) { + Get-Service | Should -Not -BeNullOrEmpty +} +``` + +`-Skip:` is evaluated during discovery, so its expression can only read state that exists at +discovery time: automatic variables, script-scope values, and `-ForEach` data bound by an +enclosing block. A `-Skip:` expression that reads a variable assigned in `BeforeAll` sees +`$null`, because `BeforeAll` does not run until execution. The test then skips +unconditionally, and a skipped test reads as a passing build. + +```powershell +# Bad - $expectedTracks is assigned in BeforeAll, so -Skip: sees $null and always skips +BeforeAll { + $expectedTracks = Get-ExpectedTrackCount +} + +It 'Reports the expected track count' -Skip:($null -eq $expectedTracks) { + (Get-Album -Name 'Example').Tracks.Count | Should -Be $expectedTracks +} +``` + +`$_` inside a `-Skip:` expression is bound by an *enclosing* `Context` or `Describe` +`-ForEach`, never by the `It`'s own `-ForEach`. PowerShell evaluates the `-Skip:` argument +before `It` receives its `-ForEach` collection, so on an `It`-level `-ForEach` the `$_` in +the skip condition is always `$null` and the condition skips every generated case, including +the ones that should have run. Put the `-ForEach` on an enclosing `Context` when the skip +condition needs to read the current item. + +Measured on Pester 6.1.0 with two cases, one of which should run: the `It`-level form skipped +both, and moving `-ForEach` to the enclosing `Context` correctly ran one and skipped the other. + +```powershell +# Good - -ForEach on the enclosing Context, so $_ is bound when -Skip: is evaluated +BeforeDiscovery { + $albums = @( + @{ Name = 'First'; ExpectedTracks = 9 } + @{ Name = 'Second'; ExpectedTracks = $null } + ) +} + +Describe 'Get-Album' { + Context 'Album <_.Name>' -ForEach $albums { + It 'Reports the expected track count' -Skip:($null -eq $_.ExpectedTracks) { + (Get-Album -Name $_.Name).Tracks.Count | Should -Be $_.ExpectedTracks + } + } +} + +# Bad - $_ is not bound yet on the It's own -ForEach, so every case skips +Describe 'Get-Album' { + It 'Reports the expected track count' -ForEach $albums -Skip:($null -eq $_.ExpectedTracks) { + (Get-Album -Name $_.Name).Tracks.Count | Should -Be $_.ExpectedTracks + } +} +``` + +Compare against `$null` explicitly in skip conditions instead of relying on truthiness. +`-not 0` is `$true`, so a legitimately configured `0` silently skips the test that was meant +to verify it. + +```powershell +# Good - only a missing value skips +Context 'Case <_.Name>' -ForEach $cases { + It 'Honors the retry limit' -Skip:($null -eq $_.RetryLimit) { + (Get-RetryPolicy).Limit | Should -Be $_.RetryLimit + } +} + +# Bad - a configured RetryLimit of 0 skips too +Context 'Case <_.Name>' -ForEach $cases { + It 'Honors the retry limit' -Skip:(-not $_.RetryLimit) { + (Get-RetryPolicy).Limit | Should -Be $_.RetryLimit + } +} +``` + +### Pester Version Pinning + +This rule is specific to Pester. Pin other dependencies normally. + +Never pin Pester itself to an exact version in a dependency manifest such as `*.depend.psd1`; +use `Version = 'latest'`. Pester 6 runs discovery for each test file separately, and resolving +`Describe` triggers PowerShell module autoloading. Autoloading always selects the highest +installed version, overriding whatever version was explicitly imported beforehand. A pin below +the version already baked into the CI runner image therefore can never be honored, and the run +fails during discovery: + +```text +Could not load file or assembly 'Pester, Version=6.0.1.0'. Assembly with same name is already loaded +``` + +This was observed twice on hosted runners: one module repository pinned `6.0.1` against an +image carrying `6.1.0` and all 19 of its test files failed to run, and another repository's CI +was red for eight days for the same reason. + +```powershell +# Good - always resolve whatever Pester the runner already has +@{ + Pester = @{ + Version = 'latest' + } +} + +# Bad - a pin below the runner's installed version can never win against autoloading +@{ + Pester = @{ + Version = '6.0.1' + } +} +``` + +`Version = 'latest'` does cost reproducibility, and a new Pester release can land in a build +that was green yesterday. That trade-off is real, but for Pester there is no alternative that +works: an exact pin below the installed version cannot be honored however it is expressed. +`Import-Module -RequiredVersion` does not rescue it either, because autoloading re-resolves +`Describe` for every test file during discovery and picks the highest installed version +regardless of what was imported first. The only way to make a lower pin stick is to remove the +higher version from the machine before discovery starts, which a dependency manifest cannot +express. The real choice is between a build that resolves the newest Pester and a build that +does not run at all. + +### Data-Driven Tests with -ForEach + +An empty or `$null` `-ForEach` collection throws in Pester 6; Pester 5 silently generated no +tests instead. The throw happens during discovery, so it kills the whole container, and a +container that dies during discovery does not increment `FailedCount`. The build stays green +while every test in that file silently disappears. + +Add `-AllowNullOrEmptyForEach` only to collections that can legitimately be empty. Leave it off +wherever an empty collection means something upstream is broken - there the throw is the signal +that is wanted. + +```powershell +# Good - an optional set of extra cases may legitimately be empty +Describe 'Optional case' -ForEach $optionalCase -AllowNullOrEmptyForEach { + It 'Runs when the case exists' { + $_ | Should -Not -BeNullOrEmpty + } +} + +# Bad - hides a glob that matched no public functions at all +Describe 'Public function' -ForEach $publicFunction -AllowNullOrEmptyForEach { + It 'Has comment-based help' { + Get-Help -Name $_.Name | Should -Not -BeNullOrEmpty + } +} +``` + +### Gating the Build on Pester Results + +Set `Run.PassThru = $true` before gating on anything. With `-Configuration` and no `PassThru`, +`Invoke-Pester` returns nothing at all, so `$testResult` is `$null`, every gate below reads +`$null` as `0`, and the build passes unconditionally - the exact failure this section exists to +prevent. + +Gate the build on `$testResult.FailedContainersCount`, not on filtering `Containers` by +`Passed`. A container that died during discovery still reports `Passed = $true`, so the obvious +filter matches nothing and silently reproduces the very failure it was written to catch. + +Gate on `FailedBlocksCount` as well. `FailedCount` counts failed tests, and a `BeforeAll` or +`AfterAll` that throws is not a test. An `AfterAll` failure is the clearest case: its tests have +already passed, so the run reports `FailedCount = 0` and `FailedContainersCount = 0` while +`FailedBlocksCount = 1`. Without that gate a broken teardown ships green. + +Also assert that tests actually ran, using `PassedCount + FailedCount`. `TotalCount` includes +tests that never ran, and skipped tests report `Executed = $true` and are not counted in +`NotRunCount`, so only passed plus failed distinguishes a suite that ran from one that did not. + +```powershell +# Good - catches failed tests, broken setup/teardown, dead containers, and an empty run +$pesterConfiguration = New-PesterConfiguration +$pesterConfiguration.Run.Path = './tests' +$pesterConfiguration.Run.PassThru = $true + +$testResult = Invoke-Pester -Configuration $pesterConfiguration +if ($testResult.FailedCount -gt 0) { + throw "$($testResult.FailedCount) test(s) failed" +} + +if ($testResult.FailedBlocksCount -gt 0) { + throw "$($testResult.FailedBlocksCount) block(s) failed in setup or teardown" +} + +if ($testResult.FailedContainersCount -gt 0) { + throw "$($testResult.FailedContainersCount) container(s) failed during discovery" +} + +if (($testResult.PassedCount + $testResult.FailedCount) -eq 0) { + throw 'No tests executed' +} + +# Bad - without Run.PassThru, Invoke-Pester returns $null and every gate below is a no-op +$testResult = Invoke-Pester -Configuration $pesterConfiguration + +# Bad - a container that failed discovery still reports Passed = $true, so this matches nothing +$failedContainer = $testResult.Containers | Where-Object { -not $_.Passed } +if ($failedContainer) { + throw 'Container failure' +} + +# Bad - TotalCount includes tests that never ran, so it hides an empty run +if ($testResult.TotalCount -eq 0) { + throw 'No tests executed' +} +``` + +`Set-ItResult -Inconclusive` interacts with the last gate. An inconclusive test executes but +lands in `InconclusiveCount` without incrementing `PassedCount` or `FailedCount`, so a suite +whose tests are all inconclusive reports `0 + 0` and trips the "No tests executed" check even +though it ran. Where inconclusive results are an expected outcome, include `InconclusiveCount` +in the sum; where they are not, leave it out so an all-inconclusive suite is caught. + +### InModuleScope Placement + +Put `InModuleScope` inside the `Context` or `It` that needs it; never wrap it around `Describe` +or `It`. Pester's own documentation advises against that enclosing placement, because a +wrapping `InModuleScope` forces the module to load during discovery rather than execution. +Combined with Pester 6 discovering each test file separately, those discovery-time imports +accumulate across files until a later file's discovery hard-errors: + +```text +Multiple script or manifest modules named 'ExampleModule' are currently loaded +``` + +Prefer the documented `InModuleScope -ModuleName -ScriptBlock { }` form inside the block +that needs module-internal access. `InModuleScope` requires the module to be loaded already - +otherwise the test fails with `No modules named 'X' are currently loaded` - so import it in +`BeforeAll`, which runs during execution rather than discovery. + +```powershell +# Good - the module loads during execution, inside the block that needs it +Describe 'Get-Thing' { + BeforeAll { + Import-Module 'ExampleModule' + } + + It 'Calls the private helper' { + InModuleScope -ModuleName 'ExampleModule' -ScriptBlock { + Get-Thing -Name 'example' | Should -Not -BeNullOrEmpty + } + } +} + +# Bad - forces a module import during discovery of every file that does this +InModuleScope 'ExampleModule' { + Describe 'Get-Thing' { + It 'Calls the private helper' { + Get-Thing -Name 'example' | Should -Not -BeNullOrEmpty + } + } +} +``` + +### Matching Test Files Cross-Platform + +`Get-ChildItem -Filter` is case-sensitive on Linux and case-insensitive on Windows. A build +script that collects test files with `-Filter` therefore finds them on Windows runners and +silently finds none on Linux ones, which reads as a passing build with zero tests. Match on +`Where-Object` with `-like`, which is case-insensitive on every platform. + +```powershell +# Good - matches on Windows and Linux runners alike +$testFiles = Get-ChildItem -Path $testPath -Recurse -File | + Where-Object { $_.Name -like '*.Tests.ps1' } + +# Bad - case-sensitive on Linux, so 'Example.tests.ps1' is never found there +$testFiles = Get-ChildItem -Path $testPath -Recurse -File -Filter '*.Tests.ps1' +``` diff --git a/instructions/update.instructions.md b/instructions/update.instructions.md index 623ed3c..4fc7231 100644 --- a/instructions/update.instructions.md +++ b/instructions/update.instructions.md @@ -29,6 +29,20 @@ Repositories control AIM behavior through `aim.config.json` in the repository ro "description": "Community-contributed instructions from GitHub" } ] + }, + "skills": { + "enabled": true, + "vendorPath": ".agents/skills", + "dependencies": [ + { + "name": "psake", + "source": "psake/psake-llm-tools", + "path": "plugins/psake/skills/psake", + "version": "v2.2.0", + "format": "skill-md", + "description": "psake build authoring (Agent Skill, agentskills.io)" + } + ] } } ``` @@ -40,6 +54,13 @@ Repositories control AIM behavior through `aim.config.json` in the repository ro - `modules.exclude` - List of modules to exclude (takes precedence over include) - `externalSources.enabled` - Enable fetching from external repositories - `externalSources.repositories` - List of external instruction sources +- `skills.enabled` - Enable vendoring declared Agent Skill (SKILL.md) dependencies into the repo +- `skills.vendorPath` - Directory skills are vendored into (default `.agents/skills`, the + cross-client Agent Skills convention) +- `skills.dependencies` - List of skills to vendor, each with `name`, `source` (repo), `path` + (skill folder within the source), `version` (tag to pin, or `latest`), `format` (`skill-md`), + and `description`. Unlike instruction modules, skills are copied to `vendorPath` (not + `instructions/`) and routed via `AGENTS.md` - see step 7 ## Update Procedure @@ -106,21 +127,50 @@ Fetching python.instructions.md from github/awesome-copilot... Fetching react.instructions.md from github/awesome-copilot... ``` -### 7. Update AGENTS.md +### 7. Handle Skill Dependencies + +If `skills.enabled` is true, vendor each declared Agent Skill (SKILL.md format) into the +repository so it travels with the code and any agent can use it - materialized like an instruction +module, not installed per-developer. Skills are NOT copied into `instructions/`; they are vendored +under `skills.vendorPath` (default `.agents/skills`), the cross-client +[Agent Skills](https://agentskills.io) convention that conforming agents discover directly. + +For each entry in `skills.dependencies`: + +1. Resolve `source` at the pinned `version` and locate the skill folder at `path` (the directory + containing `SKILL.md`). `version` is an exact tag or `latest`; `latest` means the most recent + release tag of `source` (its newest version tag when the source publishes no GitHub releases), + never the default branch's moving HEAD, so every agent vendors identical contents. +2. Copy that folder verbatim to `//` (the `SKILL.md` plus any `references/`, + `scripts/`, or `assets/`). Do not edit the vendored copy - re-sync from upstream instead. +3. **If `//` already exists, ask the user** before overwriting (same posture as + instruction files): overwrite / skip / diff. +4. Record or refresh upstream attribution and license in `/NOTICE.md`. +5. Route the skill in `AGENTS.md`: add a row to the Instruction Applicability Matrix mapping the + relevant task type to `//SKILL.md`, and list it in the "Skill Dependencies" + section. This is what makes any AGENTS-aware agent consult the skill. +6. Ensure a `CLAUDE.md` exists whose first line imports `AGENTS.md` (`@AGENTS.md`). Claude Code + reads `CLAUDE.md` - not `AGENTS.md` and not `/` - so this import is the bridge that + carries the routing into Claude Code. Preserve any Claude-specific content below the import. + +Agents that natively scan `.agents/skills/` (for example Cursor and opencode) pick the skill up +directly; the `AGENTS.md` routing plus the `CLAUDE.md` bridge covers agents that do not. + +### 8. Update AGENTS.md - Replace the HTML comment block at the top (the comment starting with `