Code Review

Playbook: code-review

Review changes to PnP PowerShell for the failure modes this repository actually ships.

Cmdlets here run unattended, against production tenants, often with tenant-wide permissions. A cmdlet that quietly does the wrong thing is worse than one that fails, because nobody finds out until the tenant is already changed. Weight findings accordingly.

Verify before reporting. A claim about behaviour that nobody ran is a guess; say so when it is one.

Language and API rules live in dotnet-standards. This playbook is about what goes wrong here specifically.

Report to the user, in the session. Never post a review, a comment, or an approval to GitHub, and never open an issue for a finding — see Human in the loop.

Layout

• src/Commands/ — cmdlet implementations, one folder per feature area
• src/Commands/Base/ — base classes, PipeBinds/ for parameter binding types
• src/Commands/Attributes/ — permission and behaviour attributes
• src/ALC/ — assembly load context that isolates private dependencies
• documentation/<Verb-PnPNoun>.md — the reference page for every cmdlet, one file each
• pages/articles/ — conceptual articles, listed in pages/articles/toc.yml
• pages/_site/ — generated site output, never edited by hand
• build/ — build and generator scripts
• CHANGELOG.md — release notes

Much of the provisioning behaviour lives in PnP Framework, a separate repository. When a root cause sits there, say so rather than accepting a workaround layered on top here, and do not let a PR claim to fix an issue whose cause it never touched.

What to look for first

Silence

The defect this repository has shipped most often is input accepted and then ignored.

• catch { } or catch { return null; } — swallowing turns a user's mistake into wrong output
• A parameter parsed into "no value given", after which the cmdlet proceeds with its default behaviour. Ignoring a -Configuration that could not be read once meant extracting an entire site instead of the one list asked for
• System.Text.Json ignores unknown members by default, so a misspelled property silently has no effect. Custom enum converters here drop values they cannot parse, case sensitively
• An unrecognised resource prefix in a permission attribute is silently classified as SharePoint, so a typo'd "garph/…" declares a bogus SharePoint scope while the real Graph requirement goes undeclared — see permissions-auditor
• A dropped value that widens what the cmdlet does deserves an error, not a warning. An empty handler list means "all handlers", so one unrecognised handler name would otherwise turn a scoped operation into a full one

Unpaged collections

GraphRequestHelper.GetResultCollection follows @odata.nextLink. GraphRequestHelper.Get does not — pointed at a collection endpoint it returns the first page only, with no error. The same applies to RequestHelper. This presents to users as "the cmdlet misses items in large tenants", which is invisible in any tenant small enough to develop against. Check every new collection fetch.

The CSOM equivalent: a query returning more than the list view threshold, or a loop that pages manually and drops the last page.

Base class

Check the base class actually matches what the cmdlet does — a tenant-admin operation on PnPWebCmdlet, or a Graph call from a SharePoint cmdlet, gets the wrong context and the wrong permission flavour. It compiles, and it fails in someone's tenant. The table in new-cmdlet is the reference.

How errors reach the user

PnPConnectedCmdlet.ProcessRecord rethrows PipelineStoppedException untouched (src/Commands/Base/PnPConnectedCmdlet.cs:57-60) and catches everything else. Two paths, and the difference is the finding:

• WriteError / ThrowTerminatingError — under -ErrorAction Stop these surface as a pipeline stop, rethrown unchanged. The ErrorRecord, its ErrorCategory and its target object all reach the user intact.
• A raw throw — hits the generic catch. Default error action: rethrown as PSInvalidOperationException with the original as inner. Under -ErrorAction Stop or SilentlyContinue: LogError → LoggingUtility.Error → WriteError(new ErrorRecord(new Exception(message), source, ErrorCategory.NotSpecified, null)). Type, inner exception, category and target object are all discarded, so everything the user needs must be in the message text. Under -ErrorAction Ignore the LogError call is skipped altogether (PnPConnectedCmdlet.cs:112-119), so the failure is swallowed with no record at all — worth remembering when a user reports a cmdlet that "does nothing and says nothing".

So a raw throw carrying a custom exception type the caller is meant to inspect is a finding — the type is not observable on that path. So is a fatal condition signalled with WriteWarning and then continuing, and a throw where ThrowTerminatingError with a real ErrorCategory and target object would have told the user which object failed.