Add Copilot instructions and code-review agent guidance#2156
Add Copilot instructions and code-review agent guidance#2156
Conversation
There was a problem hiding this comment.
Pull request overview
Adds repository-level guidance for Copilot and a dedicated AL-Go code review agent document to help enforce AL-Go conventions—especially around settings/schema documentation and review focus areas.
Changes:
- Introduces
.github/copilot-instructions.mddescribing AL-Go conventions for PowerShell, workflows, testing, security, and documentation. - Adds
.github/.agents/code-review.agent.mddefining review focus areas and repository knowledge for a code-review agent.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
.github/copilot-instructions.md |
New Copilot guidance covering conventions, security patterns, testing, and documentation requirements. |
.github/.agents/code-review.agent.md |
New code-review agent guidance defining critical/important review checks and key repo references. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - [ ] New or changed settings are documented in `Scenarios/settings.md` and reflected in `Actions/.Modules/settings.schema.json` with consistent metadata | ||
| - [ ] New public functions have appropriate comment-based help and any new workflows/user-facing behaviors are documented in scenarios/READMEs |
There was a problem hiding this comment.
Markdown checklist formatting: the last two items have an extra leading space before the dash, which renders them as a nested list and breaks the consistent checklist formatting above. Remove the extra indentation so they appear at the same level as the other checklist items.
| - [ ] New or changed settings are documented in `Scenarios/settings.md` and reflected in `Actions/.Modules/settings.schema.json` with consistent metadata | |
| - [ ] New public functions have appropriate comment-based help and any new workflows/user-facing behaviors are documented in scenarios/READMEs | |
| - [ ] New or changed settings are documented in `Scenarios/settings.md` and reflected in `Actions/.Modules/settings.schema.json` with consistent metadata | |
| - [ ] New public functions have appropriate comment-based help and any new workflows/user-facing behaviors are documented in scenarios/READMEs |
| 3. **Path traversal**: File operations that don't validate paths stay within the workspace | ||
| 4. **Missing `-recurse` on ConvertTo-HashTable**: After `ConvertFrom-Json`, always chain `| ConvertTo-HashTable -recurse` for case-insensitive access | ||
| 5. **Deprecated settings**: Flag usage of settings listed in `DEPRECATIONS.md` |
There was a problem hiding this comment.
The agent treats missing ConvertTo-HashTable -recurse as critical and implies it’s required for case-insensitive access, but -recurse only controls deep conversion and the codebase already has usages without -recurse (e.g., Actions/AL-Go-Helper.ps1 reads settings with ConvertTo-HashTable only). Consider downgrading this rule or clarifying when -recurse is actually required (nested settings access) to avoid false positives in reviews.
| - **Action pattern**: Each action lives in `Actions/<ActionName>/` with an `action.yaml` and PowerShell scripts | ||
| - **Template workflows**: `Templates/Per Tenant Extension/` and `Templates/AppSource App/` contain the workflow templates shipped to users | ||
| - **Shared modules**: `Actions/.Modules/` contains reusable PowerShell modules | ||
| - **Security checks**: `Actions/VerifyPRChanges/` validates that fork PRs don't modify protected files (.ps1, .psm1, .yml, .yaml, CODEOWNERS) |
There was a problem hiding this comment.
The VerifyPRChanges description says it blocks fork PRs from changing scripts/workflows/CODEOWNERS, but the action also blocks any changes under .github/ and .AL-Go/ paths. Consider updating this bullet to reflect the full set of protected paths/extensions so reviewers/users aren’t surprised by the additional restrictions.
| - **Security checks**: `Actions/VerifyPRChanges/` validates that fork PRs don't modify protected files (.ps1, .psm1, .yml, .yaml, CODEOWNERS) | |
| - **Security checks**: `Actions/VerifyPRChanges/` validates that fork PRs don't modify protected automation and governance files, including PowerShell scripts (.ps1, .psm1), workflow files (.yml, .yaml), CODEOWNERS, and any files under `.github/` or `.AL-Go/`. |
| ``` | ||
| - Use `try/catch/finally` with structured error propagation. | ||
| - Check `$LASTEXITCODE` after running external commands. | ||
| - Use `Write-Host "::ERROR::<message>"` for GitHub Actions error annotations. |
There was a problem hiding this comment.
The guidance uses Write-Host "::ERROR::<message>" for Actions error annotations, but the repository predominantly uses ::Error:: (and sometimes ::error::). Consider aligning the instructions to the existing convention to avoid inconsistent annotations/search patterns (e.g., see Actions/.Modules/DebugLogHelper.psm1 OutputError).
| - Use `Write-Host "::ERROR::<message>"` for GitHub Actions error annotations. | |
| - Use `Write-Host "::Error::<message>"` for GitHub Actions error annotations. |
| - Use `Write-Host "::Warning::<message>"` for non-blocking warnings. | ||
|
|
||
| ### JSON Processing | ||
| - Always use `ConvertTo-HashTable -recurse` after `ConvertFrom-Json` to ensure case-insensitive access. |
There was a problem hiding this comment.
ConvertTo-HashTable -recurse doesn’t affect case-insensitive access; PowerShell hashtables and property access are already case-insensitive. In this repo, -recurse specifically controls whether nested objects/arrays are converted (see ConvertTo-HashTable in Actions/AL-Go-Helper.ps1). Suggest rewording this bullet to describe the actual benefit (deep conversion for nested settings) to avoid misleading guidance.
| - Always use `ConvertTo-HashTable -recurse` after `ConvertFrom-Json` to ensure case-insensitive access. | |
| - Always use `ConvertTo-HashTable -recurse` after `ConvertFrom-Json` to ensure nested objects and arrays are converted to hashtables for consistent access. |
aholstrup1
left a comment
aholstrup1
left a comment
There was a problem hiding this comment.
Probably something we should've added a while ago. Thanks for adding it! 👍
| ### Important (Should Flag) | ||
| 1. **Missing tests**: New or modified functions should have corresponding Pester tests in `Tests/` | ||
| 2. **Cross-platform issues**: Hardcoded path separators, PS5-only or PS7-only constructs | ||
| 3. **Encoding omissions**: File read/write without explicit `-Encoding UTF8` | ||
| 4. **YAML permissions**: Workflows without minimal permission declarations | ||
| 5. **Missing RELEASENOTES update**: User-facing changes without a release note entry | ||
| 6. **Missing documentation for new settings**: New or changed AL-Go settings must be documented in `Scenarios/settings.md` (including purpose, type, default/required status, and which templates/workflows honor them) and represented in the settings schema (`Actions/.Modules/settings.schema.json`) with matching descriptions and correct metadata (`type`, `enum`, `default`, `required`). | ||
| 7. **Missing documentation for new functions**: New public functions (exported from modules or used as entry points) should include comment-based help (e.g., `.SYNOPSIS`, `.DESCRIPTION`, parameter help) and be described in relevant markdown documentation when they are part of the public surface. | ||
| 8. **Missing documentation for new workflows or user-facing behaviors**: New or significantly changed workflows/templates in `Templates/` must have corresponding scenario documentation (or updates) in `Scenarios/`, and new user-facing commands or actions must be documented in scenarios or `README.md`. |
There was a problem hiding this comment.
Nitpick: Consider if we should separate the different code review rules into separate files like we do internally e.g. a Style.md, Security.md etc. Might make it easier to keep track of the different rules in the long run.
Add repository-level Copilot instructions and tighten the AL-Go code-review agent to enforce documentation for settings, schema, and public functions.