fix: Test-InforcerConnection returns a boolean

[royklo]

It wrote to the host and returned nothing, so if (Test-InforcerConnection) was always false — the Test-* verb promised a predicate that was never there.

Returning a boolean alone would not have fixed it. Verified live: with Write-Error left in place, $ErrorActionPreference = 'Stop' makes the call throw instead of answering, so the predicate stays broken in exactly the strict scripts most likely to check a connection first. Both failure paths moved to Write-Warning; the guardian's error-handling rule gained a Test-* exception.

Blast radius checked before changing: no callers inside the module, none in the InforcerCommunity-MCP server. Only user scripts are affected — those reading -ErrorVariable, or relying on the throw under Stop to abort. One more worth knowing: a wrapper function that calls this as a check now emits False into its own output, so @(Get-MyWrapper) returns 2 objects where it returned 1. Migration for all three is in CHANGELOG.

Verified against the live API: valid session -> True, no session -> False + warning, bad key under Stop -> False without throwing.

Closes audit item A9. 468/0/2.

Summary of changes

Brief description of what this PR does.

Related issue

Fixes #(issue number) — or "None".

Checklist

• Tests pass locally (Invoke-Pester ./Tests/Consistency.Tests.ps1)
• Comment-based help is complete for any cmdlet I changed (synopsis, description, parameters, examples)
• I followed the consistency contract (parameter order, property names, JSON depth 100) — see CONTRIBUTING.md
• If I added or changed a cmdlet, I updated docs/CMDLET-REFERENCE.md

[Copilot]

🟡 Changes recommended

Unresolved help-text, test-coverage, versioning, and changelog issues remain.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes Test-InforcerConnection to return a Boolean predicate and report failures as warnings.

Changes:

• Implements $true/$false results and updates failure handling.
• Adds tests and updates documentation and migration guidance.
• Updates module metadata and changelog.

File summaries

File	Findings
Tests/Consistency.Tests.ps1	Moderate (2 votes): Add mocked success and API-failure coverage, including strict error-preference behavior.
README.md	No findings.
module/Public/Test-InforcerConnection.ps1	Critical (2 votes): Correct inaccurate “Progress” help text at lines 18, 46, and 50. Moderate (1 vote): Add tests for successful and failed requests, including strict preference behavior.
module/InforcerCommunity.psd1	Moderate (1 vote): Breaking contract changes require a minor version bump rather than 0.7.3.
docs/CMDLET-REFERENCE.md	Nit (3 votes): Remove or update the stale statement that no-session calls write an error.
CHANGELOG.md	Moderate (1 vote): Classify the breaking changes under a Breaking Changes heading and publish as 0.8.0.

Review details

Suppressed comments (5)

CHANGELOG.md:13

• These bullets explicitly describe consumer-visible breaking changes: the cmdlet now emits a pipeline value, switches from -ErrorVariable to -WarningVariable, and no longer throws under Stop. The repository's versioning policy requires pre-1.0 breaking changes to use a MINOR version under a Breaking Changes heading, so 0.7.3 under Bug Fixes is inconsistent; publish this as 0.8.0 and classify it accordingly.

- **`Test-InforcerConnection` now returns `$true` / `$false`.** It previously wrote to the host and returned nothing at all, so `if (Test-InforcerConnection) { ... }` was always false — the `Test-*` verb promised a predicate the cmdlet never delivered. It now returns a boolean; the host output is unchanged.
  - **Failures are warnings, not errors.** "Not connected" and a failed API call used to call `Write-Error`. That cannot coexist with a predicate: under `$ErrorActionPreference = 'Stop'` a WriteError throws instead of returning `$false`, so the fix would have been useless in exactly the strict scripts most likely to check a connection first. Both paths now use `Write-Warning`.
  - **Migration.** Anything reading `-ErrorVariable` from this cmdlet should read `-WarningVariable` instead. The sharper edge: a script running under `Stop` that called `Test-InforcerConnection` bare, relying on the throw to abort, now continues past it — check the return value instead, e.g. `if (-not (Test-InforcerConnection)) { throw 'no connection' }`. Nothing inside the module calls this cmdlet and neither does the MCP server, so the blast radius is your own scripts.

module/InforcerCommunity.psd1:3

• This changes the public contract in two breaking ways: it adds a pipeline boolean and moves failure reporting from the error stream to warnings; the changelog explicitly calls out scripts using -ErrorVariable or relying on -ErrorAction Stop as affected. The versioning policy in CHANGELOG.md:5 requires pre-1.0 breaking changes to bump the minor version, so 0.7.3 (and the matching changelog heading) should be a minor-version release.

    ModuleVersion     = '0.7.3'

module/Public/Test-InforcerConnection.ps1:46

• return $true is reached after an Invoke-RestMethod call that does not set -ErrorAction Stop. HTTP failures from this cmdlet can be non-terminating under the default preference, so the catch may be skipped and a failed or unauthorized probe can still return $true. Make the probe terminating so this predicate reliably returns $false on request failure.

    return $true

module/Public/Test-InforcerConnection.ps1:51

• Every HTTP exception is converted to $false, but Connect-Inforcer intentionally establishes sessions for valid narrow-scope keys after an app-envelope 403 on this same /beta/baselines probe (see Connect-Inforcer.ps1:219-225 and the existing narrow-scope tests). A Reports.Read-only session therefore returns false here, so the new documented reconnect pattern treats a live session as dead; preserve/inspect the response envelope or use a scope-neutral probe before returning false.

    Write-Warning "Inforcer connection test failed: $($_.Exception.Message)"
    return $false

module/Public/Test-InforcerConnection.ps1:46

• The added tests cover only the no-session branch; the new $true return and the API-failure warning/$false path are untested. This suite already uses Pester module mocks, so add a valid synthetic session with mocked Invoke-RestMethod for both success and a thrown request (including $ErrorActionPreference = 'Stop') to keep the core predicate contract regression-tested.

    return $true

• Files reviewed: 6/6 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.

[Copilot]

🟡 Changes recommended

Resolve the help/output mismatch and update the breaking-change version and changelog heading.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

module/Public/Test-InforcerConnection.ps1:51

• Returning $false for every Invoke-RestMethod exception makes a valid narrow-scope session look disconnected. This probe calls /beta/baselines, and Connect-Inforcer deliberately treats the Inforcer 403 envelope from that endpoint as proof that the key is valid but lacks Baselines.Read (see module/Public/Connect-Inforcer.ps1:210-225 and Tests/Consistency.Tests.ps1:1905-1953). With the documented reconnect pattern, such users will get False and repeatedly reconnect; preserve that status/envelope distinction or use a scope-independent probe before mapping the result to a predicate.

    Write-Warning "Inforcer connection test failed: $($_.Exception.Message)"
    return $false

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Lite