release: v0.7.0 — opt-in file writing, group membership rules, 8 bug fixes - #40
Merged
Conversation
Every call failed, single and multi-tenant, with 400 ValidationFailure
"Unspecified content type application/json is not allowed".
POST /beta/tenants/{id}/assessments/{id}/runs takes no request body and rejects
ANY Content-Type. Verified with curl: no header returns 200, application/json
and text/plain both 400, and the error echoes back whichever type was sent.
Removing the header from the hashtable is not enough - Invoke-RestMethod
supplies application/x-www-form-urlencoded on a bodyless POST, which is
rejected the same way. -ContentType '' suppresses it entirely.
Also guards the zero-result case. When every tenant failed, the run loop fell
through to renderers whose -TenantResults is mandatory, so the user got
"Cannot bind argument to parameter 'TenantResults' because it is an empty
collection" - a parameter-binding error that says nothing about why the runs
failed. One guard after the loop covers the Json, Html and Csv paths.
Verified against api-uk with two tenants: 21 checks each, matrix HTML written,
76.2% and 71.4%. Re-breaking only the Content-Type confirms the guard now
reports NoAssessmentResults and writes no file. 399 tests pass.
-SourceBaselineId scoped the source and left the destination at its full policy
set, so N baseline policies were compared against the destination's entire
estate and every destination-only policy counted as a deviation:
-SourceBaselineId 'demo - partial baseline' -DestinationTenantId 18159
before: 0.2% over 1422 items (3 source policies vs 756 destination)
after: 100% over 3 items (3 vs 3, matches Inforcer's own score)
The docstring advertised exactly that one-sided form as an example, so the
documented usage produced the misleading number.
The destination now inherits -SourceBaselineId unless -DestinationBaselineId
overrides it. When the destination cannot be scoped - it may legitimately not be
a member of the source's baseline - it falls back to the full policy set with a
warning naming the consequence, rather than erroring. An explicitly passed
-DestinationBaselineId that fails is still an error. Safe because
Select-InforcerBaselinePolicies returns null before it mutates DocData, so the
unfiltered destination survives intact, and DestinationBaselineName stays null
so neither the report header nor the filename claims a scope that was not
applied.
Also: -Tag matching nothing rendered an empty document and exited 0, which reads
as 'nothing here is tagged that way' when the likelier cause is a tag name that
does not exist. It now errors with the tags that do exist:
No policy matched tag 'Tier 1', so there is nothing to document. Tags present
in this tenant: 'Low Impact', 'MacOS Baseline - Core', ..., 'Tier 0'.
Both verified against api-uk, including the fallback branch via a forced
destination-scope failure. 399 tests pass; analyzer counts unchanged on all
three files.
Compare-InforcerEnvironments and Export-InforcerTenantDocumentation both had `$OutputPath = '.'` and an unconditional Start-Process on the rendered HTML, so neither could run without dropping a file in the caller's working directory and spawning a browser window. In a pipeline or a container that is wrong twice over: the file is litter and there is nothing to open it with. Both now: - write only when -OutputPath is given. No default. Without it the cmdlet returns the model - the comparison hashtable, or the DocModel - so a caller can read the numbers without touching disk. -OutputPath's presence IS the opt-in; a separate -Export switch would carry no information the path does not already carry. - open a browser only with -Show. BREAKING CHANGE: Compare-InforcerEnvironments and Export-InforcerTenantDocumentation no longer write to the current directory by default, and no longer open a browser. Callers relying on either must pass -OutputPath and/or -Show. Both return the model instead of System.IO.FileInfo when -OutputPath is omitted. Separately, -ExcludeOS was a no-op for every value its own documentation gave as an example. It matched only $productName (Entra, Intune, Defender, ...), while the OS lives in the category key built from primaryGroup (Windows, macOS, iOS/iPadOS, Android). 'macOS','iOS' removed 0 of 2229 items while reporting success. It now matches the category key as well as the product name, so the documented values work and passing a product name still does: full 2229 items, 61.1% -ExcludeOS macOS,iOS 1509 items, 72.3% (720 removed, was 0) -ExcludeOS Defender 2111 items, 58.9% (118 removed, unchanged) This also corrects an earlier claim of mine that -ExcludeOS worked: the item drop I attributed to it was the tenant's own data changing between runs. 399 tests pass. Analyzer gains one PSAvoidUsingWriteHost per touched file, matching the module's existing deliberate use of Write-Host for progress.
Bumps ModuleVersion to 0.7.0. The pipeline does not compute versions (Test-Version.ps1 only asserts the manifest is ahead of the Gallery), so this is set by hand; Generate-Changelog.ps1 routes the feat!/BREAKING CHANGE commit into its own Breaking Changes heading. CHANGELOG.md gains a 0.7.0 entry leading with the breaking change and its migration steps, then the four bug fixes shipped alongside it. Also corrects the versioning rule stated at the top of CHANGELOG.md. It claimed 'only breaking changes bump MAJOR', which 0.7.0 contradicts. Reworded to say what we actually do: pre-1.0, a breaking change bumps MINOR and is called out under a Breaking Changes heading, and 1.0.0 is reserved for declaring the public surface stable. docs/CMDLET-REFERENCE.md: -OutputPath rows for both affected cmdlets now say there is no default and describe the model return, and both gain a -Show row.
Every call whose alias differed from the API name only by case did nothing. The "does this alias already exist" guard uses $o.PSObject.Properties[$aliasName], which is a case-INSENSITIVE lookup, so 'ClientTenantId' found the existing 'clientTenantId' and bailed. 229 of 238 calls, silent since they were written. Deleted rather than repaired, because making them work would have been worse than leaving them broken: - PowerShell member access is already case-insensitive. $t.ClientTenantId and even $t.TENANTFRIENDLYNAME resolve today with no alias present. There was nothing to fix. - An alias IS serialised. The 9 real renames already emit both "id" and "BaselineId" in ConvertTo-Json, and two separate Export-Csv columns. 229 more would have doubled every key and column - the same value in two casings. - -OutputType JsonObject returns before this helper runs, on purpose, so the JSON surface is the raw API shape and never carried PascalCase in the first place. The guardian skill already documented the duplicate-field hazard for JsonObject output; it just was not followed through to the conclusion that it applies to every alias, not only that one path. The 9 genuine renames are untouched and verified still firing: BaselineId<-id, BaselineName<-name, PolicyId<-id, OutputFormats<-supportedOutputFormats, Parameters<-requiredParameters, Id<-runId, OutputId<-id, OutputFormat<-format, FileSize<-sizeBytes. Per-type shape fixes are untouched too - tenant licence flattening, policy PolicyName coalescing, audit metadata flattening. No behaviour change: property access, Select-Object, ConvertTo-Json, Export-Csv and every Format.ps1xml view produce identical output. The -ObjectType ValidateSet is unchanged so all 13 call sites still work; a type needing no normalisation now simply has no branch. 462 -> 173 lines. 399 tests pass, analyzer unchanged at 2.
Connect-InforcerGraph detected a missing Microsoft.Graph.Authentication and then ran Install-Module -Scope CurrentUser -Force -AllowClobber on it. -Force suppresses the untrusted-repository prompt, -AllowClobber permits overwriting commands owned by other modules, and neither was consented to. A read-only reporting module has no business mutating the machine's module state, and on a locked-down or offline host the install failed with a PowerShellGet error instead of saying what was missing. It now detects and reports, naming the install command - the same pattern Export-InforcerDocExcel already uses for ImportExcel. Reports via Write-Warning rather than Write-Error, deliberately. Every caller (Export-InforcerTenantDocumentation, Connect-Inforcer, Resolve-InforcerGraphEnrichment) already tests 'if (-not $graphCtx)' and falls back to raw ObjectIDs with a warning, which is the documented -FetchGraphData behaviour. Write-Error would have turned that graceful degradation into a hard failure for anyone running with $ErrorActionPreference = 'Stop'. ImportExcel uses Write-Error correctly because Excel export has no fallback; this does. Returns $null and clears $script:InforcerGraphConnected, matching the existing connection-failure path rather than inventing a second contract. Verified by shadowing the availability check: returns $null, emits the warning, raises 0 errors, and installs nothing. 399 tests pass.
api-feedback.md, Reports-API-Feedback.md and api-feedback-evidence/ were untracked but not ignored, so one `git add -A` would have committed them. The evidence directory holds live response bodies captured from real customer tenants, and the feedback file quotes tenant ids, policy names and baseline ids throughout. Same reasoning as the MCP repo's docs/FINDINGS.md, which was tracked for two days because a .gitignore rule was added without a `git rm --cached`. Ignoring these before they are ever staged avoids that. Verified with git check-ignore and a dry-run `git add -A`: none of the three is stageable now.
ConvertTo-InforcerMarkdown printed DocModel.BaselineName, which ConvertTo-InforcerDocModel fills with the first baseline attached to the tenant — unrelated to -Baseline. Exporting the Blueprint Library filtered to 'Tier 2 - Enhanced' produced a correctly filtered 238-policy document headed 'Baseline: ... Tier 0 - Initiate', and an unfiltered export named a baseline it was not scoped to at all. The header now reads FilterBaseline, the same field the HTML renderer already used, so it names the baseline actually filtered on and is omitted when there is none. -Tag is shown in the Markdown header too, matching HTML. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Pester's default TestResult.OutputPath, produced by Invoke-Pester -CI. Nothing in the repo reads it: build-and-test.yml takes the result from -PassThru in memory and there is no test-result publisher in the pipeline. A stale 145 KB copy from 26 Aug had been sitting untracked in the root. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The validation probe treats a 4xx carrying the Inforcer app envelope
(success/errorCode/errors) as "subscription is live, this key just lacks
the scope for /beta/baselines" — deliberately, so a key holding only
Reports.Read can still connect. An expired key returns that same
envelope, down to errorCode 'forbidden':
403 {"data":null,"errorCode":"forbidden","success":false,
"message":"A valid API key is required to access this endpoint.",
"errors":["API key has expired."]}
So it passed validation and every subsequent cmdlet failed with a 403 the
session had just promised would not happen. Measured against api-uk.dev:
an expired key returns that on 7 of 7 estate-level routes.
Neither the status code, the envelope shape nor errorCode separates the
two cases, and the API exposes no key-introspection endpoint, so errors[]
is the only signal available. The probe now reads it and refuses to
connect when it names a key-lifecycle problem. It reads errors[] only,
never the top-level message: the API serves that same generic sentence
for plain scope denials, and matching it would lock out every
narrow-scope key — the one thing this validation must never do.
Connecting on a denied probe is still correct, but now says so instead of
hiding it under -Verbose: a warning states that the subscription is live
and the key is not expired, but its scopes are unverified, so a later 403
is a scope gap rather than a bad session.
Filed with the API team as feedback item 25, asking for a distinct
errorCode for key-level failures, 401-vs-403, or GET /beta/me returning
{isActive, expiresAt, scopes[]} — any of which removes the string match.
Also carries doc catch-up that was already pending on this branch: the
membershipRule row on GroupSummary, and the -OutputPath / -Show /
-ExcludeOS descriptions for the 0.7.0 breaking changes.
Co-Authored-By: Claude Opus 5 <[email protected]>
Both were returned by GET /beta/tenants/{id}/groups/{groupId} and dropped by
the Format.ps1xml view, so the property that says what a dynamic group does,
and the one that says whether a group is editable in the cloud, were invisible
unless you knew to ask for Select-Object *.
MembershipRule is guarded by an ItemSelectionCondition and renders only when
populated, so static groups are byte-identical to before rather than gaining a
permanently blank row.
OnPremisesSyncEnabled renders the API's three states distinctly — True,
"False (cloud-only)" for null, "False (no longer syncing)" for an explicit
false. A bare boolean cannot separate "never synced" from "de-synced", and the
two mean different things when deciding where a group can be changed.
MembershipRule is NOT obtainable from the group list. The list endpoint emits
the key and leaves it null on every group, DynamicMembership ones included —
verified live: -Group on the same group id returns (user.userType -eq "Member")
where the list returns null. docs/API-REFERENCE.md claimed the opposite, having
been written off the OpenAPI snapshot without checking a response; corrected
here, along with the changelog bullet that introduced the claim. The
GroupSummary view carries the same conditional row so it lights up with no code
change if the API is fixed, marked with a ponytail: comment naming the gap.
6 Pester tests cover the view logic. Verified by mutation: collapsing the
three-state ScriptBlock to a plain if/else fails 2, dropping the rows fails 4.
408 pass across Tests/, 0 failures.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The breaking change in this release — no default -OutputPath, no browser without -Show — had no test. Nothing else in the suite asserts on where files land, so reintroducing $OutputPath = '.' or an unconditional Start-Process would have gone green. Six AST-level tests over both cmdlets: -OutputPath must declare no default, -Show must be a switch, and every Start-Process/Invoke-Item must sit inside an if ($Show) block. Parsing the param() block keeps this offline — no API key, no mocks to drift. Verified by mutation: restoring $OutputPath = '.' on Export-InforcerTenantDocumentation fails exactly one test. Also adds -Show to $expectedParameters for both cmdlets. That assertion is a documented subset check so its absence was not failing anything, but the release's headline new parameter belongs in the contract list. 414 pass across Tests/, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Half of 0.7.0's breaking change is reverted. The two halves had different justifications and only one survives scrutiny. A default -OutputPath writes a file nobody asked for, so that stays opt-in. But opening a report you *did* ask for, by passing -OutputPath, is the continuation of an explicit request rather than a surprise — and the original driver for suppressing it is gone. That driver was the MCP server invoking the cmdlet and spawning a browser; the MCP reads the REST API directly and never shells out to PowerShell. Verified: no child_process/spawn anywhere in its src/ or build/, runtime deps are the MCP SDK and zod, and src/manifest.ts says it outright — "a pointer, not an integration: the repos never call each other". -Show now defaults to (Test-InforcerInteractiveHost): true at a prompt, false when CI, TF_BUILD, GITHUB_ACTIONS, GITLAB_CI, JENKINS_URL, TEAMCITY_VERSION or BUILDKITE is set. An explicit -Show or -Show:$false always beats the detection, so the CI half of the original rationale still holds and a 50-tenant loop can still opt out with one flag. The detection is deliberately conservative: it answers "is this definitely not a build agent", not "is there definitely a browser". [Environment]::UserInteractive is true for most non-Windows pwsh sessions including scripted ones, so the CI variables carry the weight. 10 new tests: one per CI variable, one for an empty value not counting as CI, and an AST check that -Show's default is the helper rather than a literal. The existing guard that Start-Process/Invoke-Item stays inside if ($Show) is unchanged and still passes. Verified live: with CI=true the export writes its file and launches nothing; with CI unset the helper returns true. 424 pass across Tests/, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The changelog had drifted from the branch during the last few commits, and two README examples predated the breaking change. CHANGELOG: - Auto-open promoted to its own Features bullet. It was only described inside the Breaking Changes entry, where a reader scanning for new capability would never find it. - Tests section rewritten. It claimed 6 new tests and 408 passing; the release actually adds three batches (group format views, the opt-in output contract, interactive detection) for 424 passing, and the live smoke results were not recorded at all. - Documentation section notes the -Show default and the two Output sections that had gone stale. README: - The baseline example had no -OutputPath, so under 0.7.0 it writes nothing while its comment promises a document. Added the flag. - Export and Compare rows now say -OutputPath is what writes files, and the quick-start explains the interactive/CI browser behaviour. The README was the only doc still silent on the release's breaking change. docs/CMDLET-REFERENCE.md: - Both Output sections still said the report "opens in the default browser only when -Show is passed", untrue since 321507e. Verified: 424 pass across Tests/, 0 failures. README cmdlet table is 21 rows against 21 in FunctionsToExport. .psd1 0.7.0, CHANGELOG top entry 0.7.0, main 0.6.0. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Neither described anything a user can observe. The alias removal was a no-op by definition — the 229 calls it deleted never did anything, so there is no behaviour to announce — and test counts are a property of the repo, not of the release. Both are recoverable from git history and FINDINGS.md if anyone needs the reasoning. 0.7.0 keeps Breaking Changes, Features, Bug Fixes and Documentation. Earlier releases keep their own Tests sections untouched; this is not a retroactive change to shipped entries. Pure deletion — zero lines added. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
These shipped verified by hand and by a live smoke, with nothing in CI to catch a regression. Each test targets the smallest seam that encodes the fix. Invoke-InforcerAssessmentRun (2 tests) — asserts the run POST passes -ContentType "" and that no Content-Type header is set anywhere in the file. Source-level rather than a Mock: the call runs in a separate runspace via AddScript, so a module-scoped Mock cannot reach it, which makes the script text the actual contract. -ExcludeOS (4 tests) — synthetic DocModels with Windows, macOS and iOS categories under one product. Asserts a category is dropped when its KEY contains the excluded OS, that several can go at once, that only the named OS goes, and that excluding a PRODUCT name still works — that last was the only behaviour the original code had, so it is the thing most likely to be broken by a careless fix. Destination baseline inheritance (4 tests) — mocks Get-InforcerDocData and Select-InforcerBaselinePolicies, then asserts the destination is scoped to the source baseline when none is given, that an explicit -DestinationBaselineId still wins, that an inherited baseline the destination is not a member of warns and falls back rather than erroring, and that an explicit one that fails is still an error. All three mutation-verified. Reintroducing the Content-Type fails 1; reverting -ExcludeOS to product-name-only fails 2; removing the inheritance branch fails 2. 434 pass across Tests/, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
There was a problem hiding this comment.
🟢 Approval recommended
The remaining findings are minor comment/documentation wording issues, while the functional changes are broadly covered by updated Pester tests and aligned documentation.
Pull request overview
Release v0.7.0 for the InforcerCommunity PowerShell module, centered on making report/file generation explicitly opt-in while tightening several behavioral edge cases (assessment runs, baseline scoping, tag filtering, Graph dependency handling) and updating docs/tests to lock the new contracts in.
Changes:
- Make
Export-InforcerTenantDocumentationandCompare-InforcerEnvironmentsreturn models by default and only write files when-OutputPathis provided; add-Showwith interactive/CI detection viaTest-InforcerInteractiveHost. - Fix assessment execution (
Invoke-InforcerAssessmentRunsuppressesContent-Type;Invoke-InforcerAssessmentguards “all tenants failed” case) and comparison correctness (destination baseline inheritance;-ExcludeOSmatches category keys). - Expand group detail formatting (
MembershipRule, 3-stateOnPremisesSyncEnabled), remove unnecessary aliasing work, and update docs + Pester tests to prevent regressions.
File summaries
| File | Description |
|---|---|
| Tests/Renderers.Tests.ps1 | Strengthens Markdown renderer assertions for baseline header behavior. |
| Tests/Consistency.Tests.ps1 | Adds contract tests for opt-in output/show behavior plus regression tests for key bug fixes and formatting views. |
| README.md | Updates examples and cmdlet summaries to reflect opt-in output and return-type changes. |
| module/Public/Invoke-InforcerAssessment.ps1 | Adds guard to prevent empty-results rendering path from throwing binding errors. |
| module/Public/Get-InforcerGroup.ps1 | Updates help text to explain MembershipRule availability and call patterns. |
| module/Public/Export-InforcerTenantDocumentation.ps1 | Implements opt-in file writing, -Show behavior, tag no-match erroring, and updated output contract. |
| module/Public/Connect-Inforcer.ps1 | Distinguishes expired/revoked keys from scope-denied keys during probe validation; warns on unverified scopes. |
| module/Public/Compare-InforcerEnvironments.ps1 | Implements opt-in report writing, -Show behavior, improved baseline scoping semantics, and -ExcludeOS messaging. |
| module/Private/Test-InforcerInteractiveHost.ps1 | Adds CI/interactive detection helper used for -Show defaults. |
| module/Private/Invoke-InforcerAssessmentRun.ps1 | Ensures POST runs endpoint sends no Content-Type by using -ContentType "". |
| module/Private/Get-InforcerComparisonData.ps1 | Makes destination baseline inherit source baseline unless explicitly overridden; warns/falls back for non-member destination. |
| module/Private/ConvertTo-InforcerMarkdown.ps1 | Fixes Markdown header metadata to use filter fields (FilterBaseline/FilterTag) instead of tenant baseline. |
| module/Private/Connect-InforcerGraph.ps1 | Stops auto-installing Graph module; emits warning and gracefully falls back. |
| module/Private/Compare-InforcerDocModels.ps1 | Fixes -ExcludeOS to apply to category keys (where OS info actually lives). |
| module/Private/Add-InforcerPropertyAliases.ps1 | Removes redundant case-only aliasing; keeps only meaningful renames/shape fixes. |
| module/InforcerCommunity.psd1 | Bumps module version to 0.7.0. |
| module/InforcerCommunity.Format.ps1xml | Enhances group views with conditional MembershipRule and 3-state OnPremisesSyncEnabled. |
| docs/CMDLET-REFERENCE.md | Updates cmdlet docs for opt-in output, -Show, -ExcludeOS, and group formatting details. |
| docs/API-REFERENCE.md | Clarifies membershipRule behavior on list endpoint and documents 3-state onPremisesSyncEnabled. |
| CHANGELOG.md | Adds 0.7.0 entry with breaking change callouts, features, fixes, and documentation notes. |
| .gitignore | Ignores API feedback artifacts and Pester CI output file. |
Review details
- Files reviewed: 20/21 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+280
to
+284
| <!-- ponytail: dormant today. GET /beta/tenants/{id}/groups emits the membershipRule | ||
| key but always null, even for DynamicMembership groups; only the by-ID endpoint | ||
| fills it. The condition means this costs a null check and renders nothing, and | ||
| lights up on its own if the API starts populating the list. Drop it if the API | ||
| removes the key instead. --> |
Both have existed since the parameters were added and are described in Get-Help, but the CMDLET-REFERENCE parameter table skipped them — so the reference listed 8 of the cmdlet's 10 parameters. Found by diffing every cmdlet's real parameter set against its section in the reference, in both directions. Pre-existing on main; not introduced by this release. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
There was a problem hiding this comment.
🔵 Needs a closer look
The PR introduces broad, breaking behavior changes across multiple cmdlets (output contracts, auth validation, and execution behavior), so it warrants final human review despite only minor nits found.
Review details
Suppressed comments (3)
module/Public/Export-InforcerTenantDocumentation.ps1:455
- The comment above the -Show gate is grammatically incomplete ("Doing it containers...") and is hard to understand; it looks like a truncated sentence.
# -Show defaults to on interactively, off in CI (Test-InforcerInteractiveHost). Doing it
# containers, where there is nothing to open it with.
module/Public/Compare-InforcerEnvironments.ps1:354
- The comment above the -Show gate is grammatically incomplete ("Doing it run...") and is hard to understand; it looks like a truncated sentence.
# -Show defaults to on interactively, off in CI (Test-InforcerInteractiveHost). Doing it
# run, including inside CI containers where there is nothing to open it with.
module/InforcerCommunity.Format.ps1xml:284
- The XML comment starts with "ponytail", which is unclear jargon in this context and makes the rationale harder to follow for future maintainers.
<!-- ponytail: dormant today. GET /beta/tenants/{id}/groups emits the membershipRule
key but always null, even for DynamicMembership groups; only the by-ID endpoint
fills it. The condition means this costs a null check and renders nothing, and
lights up on its own if the API starts populating the list. Drop it if the API
removes the key instead. -->
- Files reviewed: 20/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
Both read "…(Test-InforcerInteractiveHost). Doing it" followed by an orphaned fragment — "run, including inside CI containers…" and "containers, where there is nothing to open it with." Self-inflicted in 321507e: a scripted string replacement swapped the first line of a two-line comment and left the second line dangling. Caught in PR review. Rewritten as complete sentences that state the current behaviour rather than the old rationale. No code change; 434 tests still pass. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of concrete issues to address in changed code (a potential null/empty handling bug in the new -ExcludeOS matching loop, and an unprofessional/unclear term in a new format XML comment).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 20/21 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+432
to
+434
| foreach ($ep in $ExcludeOS) { | ||
| if ($catLower -match [regex]::Escape($ep.ToLowerInvariant())) { $skipCategory = $true; break } | ||
| } |
The suite runs on GitHub Actions, which sets GITHUB_ACTIONS=true. The tests manipulated one detector variable at a time and left the runner's own in place, so Test-InforcerInteractiveHost kept returning false no matter what they set. Two consequences, one visible and one not: - "ignores an empty CI variable" FAILED — it cleared CI, GITHUB_ACTIONS was still set, the helper correctly said false, and the test wanted UserInteractive. Green locally, red on the runner. - The seven "returns false when <Var> is set" tests PASSED for the wrong reason. Any of them would have passed with the variable under test ignored entirely, because the runner's own variable was doing the work. All of them now clear every detector first and set only the one under test, so a false result can only come from that variable. Adds a third case for "no CI variable set at all". The production code was never wrong — the helper behaved correctly in both environments the whole time. Verified in both: 435 pass / 0 fail with GITHUB_ACTIONS+CI set, and with every detector cleared. Mutation-verified: making the helper always return true now fails all 7 detector tests, where previously it would have failed none of them on a CI runner. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…mbershipRule The 2026-09-04 drift sync merged from main adds membershipRule to tenantGroupSummary in openapi-snapshot.json, described as "only populated for groups whose groupTypes contains DynamicMembership". That is not what the endpoint does. Verified live on 2026-09-09 against two unrelated tenants: 0 of 10 and 0 of 7 DynamicMembership groups carried a rule on the list, while the by-ID endpoint returned one for each. The reference already said "always null on this endpoint"; it now says so against the schema explicitly, with both sample sizes and a pointer to API feedback item 26. A reader diffing the snapshot will otherwise reasonably conclude our note is stale. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a breaking behavioral contract across core cmdlets and broad multi-file changes that warrant final human verification despite strong test additions.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
CHANGELOG.md:13
- This changelog entry cites
src/manifest.ts, but that path does not exist in this repository. As written, it looks like a broken reference and may confuse readers; consider removing the file-path citation or updating it to a path that exists in this repo.
module/Private/Compare-InforcerDocModels.ps1:436 - This -ExcludeOS category filter uses a regex (-match with Regex.Escape) plus per-iteration ToLowerInvariant. Since the intent is a case-insensitive substring check, using IndexOf with OrdinalIgnoreCase avoids regex overhead and is clearer about intent.
- Files reviewed: 20/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Releases 0.7.0. 16 commits, 21 files, +829/−395.
mainis 0.6.0.Compare-InforcerEnvironmentsandExport-InforcerTenantDocumentationno longer write files by default.Both carried
$OutputPath = '.', so every run dropped a file into the caller's working directory whether or not anyone asked.-OutputPathis now the opt-in, and without it the cmdlets return the model — the comparison hashtable, the DocModel — instead ofSystem.IO.FileInfo. That makes it possible to read an alignment score without producing an artefact.Migration: add
-OutputPath <dir>to any call that relied on files appearing. Scripts consuming theFileInforeturn value need it to keep that return type. Nothing is needed for the browser — see below.Features
HTML reports open automatically again, except on CI.
-Showdefaults toTest-InforcerInteractiveHost: true at a prompt, false whenCI,TF_BUILD,GITHUB_ACTIONS,GITLAB_CI,JENKINS_URL,TEAMCITY_VERSIONorBUILDKITEis set.-Showforces it,-Show:$falsesuppresses it, and an explicit value always beats the detection.This half of the original opt-in change was reverted deliberately. The two halves had different justifications: a default
-OutputPathwrites a file nobody asked for, but opening a report you did ask for is the continuation of an explicit request. The original driver for suppressing it — the MCP server invoking the cmdlet and spawning a browser — no longer exists; the MCP reads the REST API directly and never shells out to PowerShell.Get-InforcerGroup -GroupshowsMembershipRuleandOnPremisesSyncEnabled. Both were returned by the API and silently dropped by the format view.MembershipRulerenders only when populated, so static groups are unchanged.OnPremisesSyncEnableddistinguishes all three API states —True,False (cloud-only)fornull,False (no longer syncing)for an explicitfalse— because a bare blank cannot separate "never synced" from "de-synced".Bug fixes
Invoke-InforcerAssessmentcould never reach the API. Every call returned400 ValidationFailure. The endpoint takes no body and rejects anyContent-Type; dropping the header was not enough becauseInvoke-RestMethodsuppliesapplication/x-www-form-urlencodedon a bodyless POST.Invoke-InforcerAssessment -MultiTenantcrashed instead of reporting when every tenant failed.-SourceBaselineIdreported a meaningless alignment score. Scoping one side compared N baseline policies against the destination's entire estate: 0.2% where Inforcer itself reports 100%.-ExcludeOSwas a no-op for every value its own docs gave as an example. It matched product names only, while the OS lives in the category key.-FetchGraphDatasilently installed a module with-Force -AllowClobberand no consent.Connect-InforcerreportedConnectedfor an expired API key, after which every cmdlet failed 403.Export-InforcerTenantDocumentation -Tagrendered an empty document when nothing matched.Verification
434 Pester tests pass, 0 failures (was 394 on
main).New coverage targets behaviour this release changed that nothing else asserted on — the opt-in output contract, the interactive detection, the group format views, and the three fixes that previously had live verification only. Every new guard is mutation-verified: each was broken deliberately and confirmed to fail. Reintroducing
$OutputPath = '.'fails 1; reverting-ExcludeOSto product-name-only fails 2; removing baseline inheritance fails 2; reintroducing the assessmentContent-Typefails 1; collapsing the three-state sync rendering fails 2.Live smoke against a real tenant (21 groups, 10 dynamic): 14 pass, 0 fail. Covers auth including refusal of a bogus key, group list/by-ID rendering, the alias refactor, and the output contract end to end — no
-OutputPathwrites nothing and returns the model, with it writes a 2.7 MB HTML and returnsFileInfo, nothing launches under CI, and all 7 CI variables suppress opening.Known gaps
All pre-existing on
main, none introduced here:-OutputType JsonObjectexample, which the docs rule requires.Format-InforcerErrorDetailuses-Depth 5where the contract names onlyFormat.ps1xmlas a carve-out.Test-InforcerConnectionwrites to the host and returns nothing despite theTest-*verb, soif (Test-InforcerConnection)is always false. Fixing it is arguably breaking, so it wants a decision rather than a quiet change.