Skip to content

Commit ed95e9f

Browse files
gautamdshethgautamdsheth
authored
Feature: added some agents, skills and MCP server to the repo (#5439)
* Feature: added some agents, skills and MCP server to the repo * Fix review comments * fix review comments * Fix review comments * more review comments * more review comments * more review comments * more review comments * some more review comments * last round * last one hopefully
1 parent 2e87374 commit ed95e9f

35 files changed

Lines changed: 2224 additions & 467 deletions

‎.agents/README.md‎

Lines changed: 282 additions & 0 deletions
Large diffs are not rendered by default.

‎.agents/skills/api-surface-diff/SKILL.md‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
---
2+
name: api-surface-diff
3+
description: Compare the public cmdlet surface of the current branch against dev - names, aliases, parameters, types, mandatory flags, parameter sets, output types, permissions - and classify each change as breaking, behavioural or additive. Use before opening a PR, when reviewing one, or when deciding release impact and changelog wording.
4+
---
5+
6+
# Playbook: api-surface-diff
7+
8+
Compare the public cmdlet surface of a branch against `dev` and classify what changed.
9+
10+
## Why
11+
12+
For a PowerShell module the public surface *is* the contract, and it is spread over 800 files, so a
13+
breaking change arrives as an innocuous-looking one-line diff. Renaming a parameter, tightening a
14+
type, or adding `Mandatory = true` breaks every script in the wild that used it. Nothing in CI
15+
catches this; a human diff review reliably misses it.
16+
17+
The output feeds two decisions: whether the change belongs in a major release, and what the
18+
`CHANGELOG.md` entry must say.
19+
20+
> Read-only. Propose the changelog line and the release call; **never open the PR** — see
21+
> [Human in the loop](../../../AGENTS.md#human-in-the-loop).
22+
23+
## The surface
24+
25+
For every cmdlet class, extract:
26+
27+
- **Cmdlet name** — from `[Cmdlet(Verbs*.X, "PnPY")]`
28+
- **Aliases** — `[Alias(...)]` on the class
29+
- **Output type** — `[OutputType(typeof(T))]`
30+
- **Parameter sets** — every distinct `ParameterSetName`, and `DefaultParameterSetName`
31+
- **Per parameter**: name, type, `Mandatory`, `Position`, `ValueFromPipeline`,
32+
`ValueFromPipelineByPropertyName`, parameter set membership, `[Alias]`, `[ValidateSet]` values
33+
- **Permission attributes** — a narrowed scope is a surface change too: a connection that worked
34+
before may now be rejected
35+
36+
Build this for `dev` and for the branch, then diff the two structures. Diff the *extracted surface*,
37+
not the text — a moved method or reordered attribute is noise.
38+
39+
```
40+
git fetch origin dev
41+
git diff --name-only origin/dev...HEAD -- 'src/Commands/**/*.cs'
42+
```
43+
44+
Use the three-dot form so you compare against the merge base, not a moving `dev`.
45+
46+
## Classification
47+
48+
Apply one test, from the repository's own review standard:
49+
50+
> **Does any correct usage behave differently?**
51+
52+
Correct usage means an invocation that was working as documented. If only previously-broken usage
53+
changes — a parameter that was never honoured, an invocation that already threw — it is a fix.
54+
55+
**Breaking** (major release)
56+
- Cmdlet or parameter removed or renamed without an `[Alias]` preserving the old name
57+
- Parameter becomes `Mandatory`, or moves out of the default parameter set
58+
- Parameter type narrowed, or `[ValidateSet]` values removed
59+
- Positional parameter renumbered, or positional binding removed
60+
- Parameter sets restructured so a previously valid combination no longer binds
61+
- Output type changed such that a property scripts read is gone
62+
- Required permissions widened — an existing app registration stops being sufficient
63+
64+
**Behavioural** (`Changed` in the changelog, minor release)
65+
- Same signature, different result, warning, or error for the same input
66+
- Default value changed
67+
- A new confirmation prompt (`ShouldProcess`) on a path that used to run unattended — call this out
68+
explicitly, it breaks automation without changing the signature
69+
70+
**Additive** (`Added`)
71+
- New cmdlet, new optional parameter, new parameter set that does not disturb existing binding
72+
- New alias
73+
74+
A rename **with** an `[Alias]` for the old name is additive, and this repository requires that alias.
75+
A rename without one is breaking; say so and name the alias that would fix it.
76+
77+
## Reporting
78+
79+
Three sections — Breaking, Behavioural, Additive — most consequential first. For each entry:
80+
81+
- `Verb-PnPNoun`, the member, `file.cs:line`
82+
- One sentence on what changed
83+
- **The invocation that changes**, concretely:
84+
`Get-PnPFoo -Bar "x"` — bound positionally before, now requires `-Bar` by name
85+
- The suggested `CHANGELOG.md` line, in this repo's style: cmdlet names in backticks, ending with a
86+
link to the PR or issue
87+
88+
Then state the release implication in one line: additive only, or a `Changed` entry, or a genuine
89+
major-release break.
90+
91+
Do not label something breaking on the strength of a diff. Name the usage that breaks, or classify
92+
it lower.

‎.agents/skills/cmdlet-scaffolder/SKILL.md‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
---
2+
name: cmdlet-scaffolder
3+
description: Generate a new PnP PowerShell cmdlet modelled on an existing sibling - class with the right base class and permission attributes, the documentation page, and the changelog entry. Use when adding a cmdlet for a Graph or CSOM API. Output is a draft needing verification against a real tenant.
4+
---
5+
6+
# Playbook: cmdlet-scaffolder
7+
8+
Generate a new cmdlet — class, attributes, documentation page and changelog entry — modelled on an
9+
existing sibling.
10+
11+
> **The output is a draft.** You cannot call the API or run the cmdlet. Everything about the request
12+
> shape, the response fields and the permission scopes is inferred. Hand it over labelled as such,
13+
> with the inferences listed. A plausible, well-formed, subtly wrong cmdlet is the specific failure
14+
> mode this playbook exists to avoid.
15+
16+
Conventions live in [`new-cmdlet`](../new-cmdlet/SKILL.md); language rules in
17+
[`dotnet-standards`](../dotnet-standards/SKILL.md). This playbook is the procedure.
18+
19+
> **Never commit, push, or open a PR.** Leave the work in the tree and hand it over — see
20+
> [Human in the loop](../../../AGENTS.md#human-in-the-loop).
21+
22+
## 1. Establish the request
23+
24+
Before writing anything, pin down: cmdlet name (`Verb-PnPNoun`, approved verb, singular noun), the
25+
API being wrapped, the parameters, and the output shape. If the verb or noun is unsettled, ask —
26+
renaming later requires an `[Alias]` and a changelog entry.
27+
28+
Check the name is not already taken, including as an alias:
29+
30+
```
31+
grep -rn 'Cmdlet(Verbs[A-Za-z]*\.[A-Za-z]*, "PnPTheNoun")' src/Commands --include=*.cs
32+
grep -rn '\[Alias(' src/Commands --include=*.cs
33+
```
34+
35+
## 2. Choose the sibling
36+
37+
**This is the most important step.** Find the closest existing cmdlet: same folder, same base class,
38+
same API, same verb. Prefer a recently modified one — helper signatures have evolved and old call
39+
shapes survive in the tree.
40+
41+
```
42+
git log --diff-filter=M --name-only -20 -- src/Commands/<Area>
43+
```
44+
45+
Read the sibling's class *and* its `documentation/*.md` page in full. You are matching a house style,
46+
not producing generic C#. Say which sibling you used.
47+
48+
## 3. Resolve the API
49+
50+
Use the **Microsoft Learn MCP server** for the endpoint, its request/response shape and its
51+
least-privilege permissions, delegated and application. Do not answer from memory — this is where
52+
generated cmdlets go wrong, and it is exactly what the permission attributes encode.
53+
54+
Record the Learn URL for each claim; it goes in the handover.
55+
56+
## 4. Write the class
57+
58+
Per [`new-cmdlet`](../new-cmdlet/SKILL.md): correct base class, `[Cmdlet]`, `[OutputType]`, permission
59+
attributes, PipeBind parameters with validation attributes, `ExecuteCmdlet()` override.
60+
61+
- Reuse existing PipeBinds and models. Only add a new model if nothing fits, one type per file,
62+
enums under `src/Commands/Enums/`.
63+
- Graph collections: `GetResultCollection`, not `Get`.
64+
- CSOM: `ExecuteQueryRetry()`.
65+
- Destructive or overwriting: `SupportsShouldProcess` plus an actual `ShouldProcess` call and
66+
`-Force`.
67+
- Error messages into `Resources.resx`, not string literals.
68+
69+
## 5. Write the documentation page
70+
71+
`documentation/<Verb-PnPNoun>.md`, structure copied from the sibling page:
72+
73+
- Front matter — `Module Name`, `title`, `schema: 2.0.0`, `applicable`, `external help file`,
74+
`online version` slug matching the cmdlet name exactly
75+
- `## SYNOPSIS` — a **Required Permissions** block stating the same scopes as the attributes with the
76+
same delegated/application split, then a one-line summary. **Omit the block entirely** if the
77+
cmdlet carries `ApiPermissionsNotRequired`; adding one there contradicts the attribute and the
78+
existing pages
79+
- `## SYNTAX` — one ` ```powershell ` block per parameter set
80+
- `## DESCRIPTION`
81+
- `## EXAMPLES` — at least one, realistic, each followed by a sentence explaining it
82+
- `## PARAMETERS` — **alphabetical**, one ` ```yaml ` block per parameter and **nothing else fenced
83+
in this section**, including the standard `-Connection` and `-Verbose` blocks copied verbatim from
84+
the sibling
85+
- `## RELATED LINKS`
86+
87+
The YAML fields must agree with the attributes exactly — see [`docs-sync`](../docs-sync/SKILL.md) for the
88+
field mapping.
89+
90+
## 6. Changelog
91+
92+
One line under `[Current nightly]` → `Added`, cmdlet name in backticks, ending with a link to the PR
93+
or issue. Match the surrounding entries' tone: what it does and why someone would use it, not "added
94+
new cmdlet".
95+
96+
## 7. Build
97+
98+
```
99+
dotnet build src/PnP.PowerShell.sln
100+
```
101+
102+
Warning-clean. Do not touch `src/Tests`.
103+
104+
## 8. Hand over
105+
106+
Report:
107+
108+
- Files created or changed
109+
- **The sibling you modelled on**
110+
- **Inferred, not verified** — every API shape, response field and permission scope, each with the
111+
Learn URL it came from
112+
- **The invocation a maintainer should run against a tenant to verify**, including which connection
113+
type (delegated and app-only both, where the cmdlet supports both)
114+
- Anything you could not resolve and what would settle it
115+
116+
Do not describe the result as tested, working, or verified. It has been compiled, and that is all.

‎.agents/skills/code-review/SKILL.md‎

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
---
2+
name: code-review
3+
description: Review changes to PnP PowerShell for the failure modes this repository actually ships - silently ignored input, unpaged Graph collections, the wrong base class, permission attributes that do not match the API called, culture and cross-platform bugs, and missing documentation or changelog updates. Use when reviewing a diff, a PR, or uncommitted changes.
4+
---
5+
6+
# Playbook: code-review
7+
8+
Review changes to PnP PowerShell for the failure modes this repository actually ships.
9+
10+
Cmdlets here run unattended, against production tenants, often with tenant-wide permissions. A cmdlet
11+
that quietly does the wrong thing is worse than one that fails, because nobody finds out until the
12+
tenant is already changed. Weight findings accordingly.
13+
14+
Verify before reporting. A claim about behaviour that nobody ran is a guess; say so when it is one.
15+
16+
Language and API rules live in [`dotnet-standards`](../dotnet-standards/SKILL.md). This playbook is about
17+
what goes wrong here specifically.
18+
19+
> Report to the user, in the session. **Never post a review, a comment, or an approval to GitHub**,
20+
> and never open an issue for a finding — see
21+
> [Human in the loop](../../../AGENTS.md#human-in-the-loop).
22+
23+
## Layout
24+
25+
- `src/Commands/` — cmdlet implementations, one folder per feature area
26+
- `src/Commands/Base/` — base classes, `PipeBinds/` for parameter binding types
27+
- `src/Commands/Attributes/` — permission and behaviour attributes
28+
- `src/ALC/` — assembly load context that isolates private dependencies
29+
- `documentation/<Verb-PnPNoun>.md` — the reference page for every cmdlet, one file each
30+
- `pages/articles/` — conceptual articles, listed in `pages/articles/toc.yml`
31+
- `pages/_site/` — generated site output, never edited by hand
32+
- `build/` — build and generator scripts
33+
- `CHANGELOG.md` — release notes
34+
35+
Much of the provisioning behaviour lives in **PnP Framework**, a separate repository. When a root
36+
cause sits there, say so rather than accepting a workaround layered on top here, and do not let a PR
37+
claim to fix an issue whose cause it never touched.
38+
39+
## What to look for first
40+
41+
### Silence
42+
43+
The defect this repository has shipped most often is input accepted and then ignored.
44+
45+
- `catch { }` or `catch { return null; }` — swallowing turns a user's mistake into wrong output
46+
- A parameter parsed into "no value given", after which the cmdlet proceeds with its default
47+
behaviour. Ignoring a `-Configuration` that could not be read once meant extracting an entire site
48+
instead of the one list asked for
49+
- `System.Text.Json` ignores unknown members by default, so a misspelled property silently has no
50+
effect. Custom enum converters here drop values they cannot parse, case sensitively
51+
- An unrecognised resource prefix in a permission attribute is silently classified as **SharePoint**,
52+
so a typo'd `"garph/…"` declares a bogus SharePoint scope while the real Graph requirement goes
53+
undeclared — see [`permissions-auditor`](../permissions-auditor/SKILL.md)
54+
- A dropped value that *widens* what the cmdlet does deserves an error, not a warning. An empty
55+
handler list means "all handlers", so one unrecognised handler name would otherwise turn a scoped
56+
operation into a full one
57+
58+
### Unpaged collections
59+
60+
`GraphRequestHelper.GetResultCollection` follows `@odata.nextLink`. `GraphRequestHelper.Get` does
61+
not — pointed at a collection endpoint it returns the **first page only, with no error**. The same
62+
applies to `RequestHelper`. This presents to users as "the cmdlet misses items in large tenants",
63+
which is invisible in any tenant small enough to develop against. Check every new collection fetch.
64+
65+
The CSOM equivalent: a query returning more than the list view threshold, or a loop that pages
66+
manually and drops the last page.
67+
68+
### Base class
69+
70+
Check the base class actually matches what the cmdlet does — a tenant-admin operation on
71+
`PnPWebCmdlet`, or a Graph call from a SharePoint cmdlet, gets the wrong context and the wrong
72+
permission flavour. It compiles, and it fails in someone's tenant. The table in
73+
[`new-cmdlet`](../new-cmdlet/SKILL.md) is the reference.
74+
75+
### How errors reach the user
76+
77+
`PnPConnectedCmdlet.ProcessRecord` rethrows `PipelineStoppedException` untouched
78+
(`src/Commands/Base/PnPConnectedCmdlet.cs:57-60`) and catches everything else. Two paths, and the
79+
difference is the finding:
80+
81+
- **`WriteError` / `ThrowTerminatingError`** — under `-ErrorAction Stop` these surface as a pipeline
82+
stop, rethrown unchanged. The `ErrorRecord`, its `ErrorCategory` and its target object all reach
83+
the user intact.
84+
- **A raw `throw`** — hits the generic catch. Default error action: rethrown as
85+
`PSInvalidOperationException` with the original as inner. Under `-ErrorAction Stop` or
86+
`SilentlyContinue`: `LogError` → `LoggingUtility.Error` → `WriteError(new ErrorRecord(new
87+
Exception(message), source, ErrorCategory.NotSpecified, null))`. Type, inner exception, category
88+
and target object are **all discarded**, so everything the user needs must be in the message text.
89+
Under `-ErrorAction Ignore` the `LogError` call is skipped altogether
90+
(`PnPConnectedCmdlet.cs:112-119`), so the failure is **swallowed with no record at all** — worth
91+
remembering when a user reports a cmdlet that "does nothing and says nothing".
92+
93+
So a raw `throw` carrying a custom exception type the caller is meant to inspect is a finding — the
94+
type is not observable on that path. So is a fatal condition signalled with `WriteWarning` and then
95+
continuing, and a `throw` where `ThrowTerminatingError` with a real `ErrorCategory` and target object
96+
would have told the user which object failed.
97+
98+
### Cmdlet conventions
99+
100+
- `Verb-PnPNoun`, approved verbs, correct base class
101+
- `ParameterSpecified(nameof(X))` distinguishes "not supplied" from "supplied as default"
102+
- Reference-typed parameters that are dereferenced in `ExecuteCmdlet` need `[ValidateNotNull]`,
103+
otherwise `-Param $null` is a `NullReferenceException`
104+
- Permission attributes must match the APIs the cmdlet actually calls, in both directions —
105+
over-declaring forces users to grant access the cmdlet never uses
106+
- Destructive or overwriting behaviour needs `ShouldProcess`, with `-Force` bypassing only a
107+
secondary `ShouldContinue`. **`Force || ShouldProcess(...)` is a defect**: `-Force` short-circuits
108+
the `||`, `ShouldProcess` is never called, and `-Force -WhatIf` performs the operation instead of
109+
simulating it. `Force || ShouldContinue(...)` is the correct, repo-standard form
110+
- A renamed cmdlet keeps its old name as `[Alias]`
111+
112+
### Cross-platform
113+
114+
.NET 8 and PowerShell 7.4+ on Windows, Linux and macOS.
115+
116+
- No Windows-only path assumptions, no backslash string surgery
117+
- `Environment.NewLine` (what `StringBuilder.AppendLine` writes) mixed with hardcoded `\r\n` makes
118+
generated files churn purely from changing OS
119+
- Format dates and numbers with `CultureInfo.InvariantCulture`. A custom format string like
120+
`"yyyy-MM-ddTHH:mm:ssZ"` takes its separators from the current culture and produces
121+
`13.53.41` under some locales, which is not a valid `xsd:dateTime`
122+
- New package references have ALC consequences: the module assembly and CSOM live in `Core`,
123+
every other dependency is private and goes to `Common`
124+
125+
## Documentation and changelog
126+
127+
A parameter added, renamed, or changed in behaviour requires its `documentation/<Cmdlet>.md` updated
128+
in the same PR. A new cmdlet requires a new page; a removed cmdlet requires its page deleted.
129+
130+
- `## PARAMETERS` sections carry only the platyPS ` ```yaml ` metadata blocks. Other fenced blocks
131+
there risk the help build; put examples under `## EXAMPLES` with ` ```powershell ` fences
132+
- Parameter subsections are listed alphabetically
133+
- Conceptual content belongs in `pages/articles/` with front matter, registered in `toc.yml`, not in
134+
`documentation/`, which is cmdlet reference only
135+
136+
`CHANGELOG.md` entries go under `[Current nightly]` in `Added`, `Changed`, `Fixed` or `Removed`,
137+
each naming the affected cmdlets in backticks and linking its PR. A change in behaviour belongs
138+
under `Changed` even when it fixes a bug, so it appears in the release notes people read before
139+
upgrading. A PR with no changelog entry is an incomplete PR — but the file header says it is owner
140+
maintained and maintainers do add entries, so raise it as a gap, not a blocker.
141+
142+
## Breaking changes
143+
144+
Ask one question: **does any correct usage behave differently?**
145+
146+
If only previously-broken usage changes — a configuration that was never honoured, an invocation that
147+
already threw — it is a fix, and it belongs in a minor release with a `Changed` entry. Reserve a major
148+
release for changes that break usage which was working as documented. Say which of the two a PR is,
149+
and name the invocation that changes, rather than labelling it breaking on the strength of a diff.
150+
[`api-surface-diff`](../api-surface-diff/SKILL.md) has the full classification.
151+
152+
## Reporting
153+
154+
Lead with the finding, not the file tour. For each one give the location, one sentence on the defect,
155+
and a concrete failure scenario: the input, and what the user gets instead of what they expected.
156+
Rank by consequence. If a check could not be run, say what would settle it.

0 commit comments

Comments
 (0)