Skip to content

fix(machine-health): honor service_exclusions and drop the unread noise_allowlist - #9260

Merged
kyle-sexton merged 4 commits into
mainfrom
fix/machine-health-check-overrides
Oct 23, 2026
Merged

kyle-sexton merged 4 commits into
mainfrom
fix/machine-health-check-overrides

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

No related issue: owner-approved follow-up T-CHALLENGE-OVERRIDES from the machine-health checkup extension plan (decided 2026-10-21).

Summary

approvals.json documented two check_overrides keys that no script read. This wires the one that is a safety control and stops documenting the other. services.service_exclusions now keeps Restart-StoppedService away from the listed services. event-log-errors.noise_allowlist is no longer documented, and the schema still accepts it so existing files validate.

Fix

  • Restart-StoppedService.ps1 takes -ExcludedService. A target whose name matches an entry exactly, ignoring case, is skipped before any Get-Service or Start-Service call, gets no attempt record, and is reported on the information stream. The orchestrator passes the list through the new Get-ServiceExclusion (it returns an empty list when any part of the path is absent) and logs each skip as remediation_info.
  • noise_allowlist is removed from approvals.md and the Test-EventLogErrors.ps1 comment. The schema keeps the property but describes it as ignored: the schema is strict (additionalProperties: false), so dropping it would fail validation for any existing file that holds it.
  • The approvals.schema.json top-level description no longer invites hand edits. Writes go through /machine-health:setup. The exception is service_exclusions, which setup cannot write yet (#9258).

Verification

  • Pester suite on the merged head: 1419 passed, 0 failed, 1 skipped. Seven new tests cover Restart-StoppedService exclusions (an excluded stopped service is never started, the others are) and Get-ServiceExclusion (absent at each level returns an empty list).
  • The new remediation tests fail on main at parameter binding (-ExcludedService does not exist there), not at a restarted service. No test harness drives the orchestrator's remediation dispatch, so the end-to-end wiring is covered by reading the call site, not by a test.
  • check-shell-portability.sh origin/main, markdownlint, typos and the changelog-fragment check pass.
  • Left out: a setup action to add and remove exclusions, filed as #9258.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 4 commits October 22, 2026 19:43
check_overrides.services.service_exclusions was documented as the list of
services Restart-StoppedService must never touch, but nothing read it.

Restart-StoppedService takes -ExcludedService and skips any target whose
name equals an entry (case-insensitive) before Get-Service or
Start-Service, with no attempt record, reporting the skip on the
information stream. Get-ServiceExclusion reads the list from the loaded
approvals state, returning an empty list when any level is absent. The
orchestrator passes the list and writes each skip to the run log.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No check reads check_overrides.event-log-errors.noise_allowlist, so
approvals.md and the Test-EventLogErrors comment no longer present it as
a per-host extension.

approvals.json stays at schema_version 1.0. The schema is strict
(additionalProperties: false), so noise_allowlist stays an accepted
property, now described as ignored, and an existing file that holds it
still validates.

The schema description also no longer invites hand edits: approvals.json
is written by /machine-health:setup (Set-ApprovalState) and the one-time
first-run migration.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Setup cannot write it yet (#9258), so the schema's no-hand-edit line excepts it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 22, 2026 23:51
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-22T23:53:16.222618Z f95e4e6 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

Copy link
Copy Markdown
Contributor

claude-security-review reviewed f95e4e66c6b575be3f1f53bbe38b1b77164cce96: 0 finding(s), 0 as inline comments.

Security review: PR #9260 (head f95e4e6)

Result: no security issues found. 0 findings, no inline comments posted.

Scope: The PR makes Restart-StoppedService honor approvals.json check_overrides.services.service_exclusions, and drops the docs for the unread noise_allowlist. I read the full diff (10 files: PowerShell scripts, schema description text, docs, tests, changelog fragment).

Analysis

  • The change only narrows what the remediation touches. A matching service is skipped before any Get-Service/Start-Service call. -in is case-insensitive and exact-match. An empty or missing list changes nothing, so no fail-open path is added.
  • Invoke-Remediation forwards arguments by hashtable splatting (@remArgs). The finding JSON and exclusion list are passed as values, never interpolated into a command string, so there is no injection surface.
  • The new remediation_info log line includes the service name. Forging a log line would need control over local service names, and no trust boundary is crossed.
  • The exclusion list comes from the per-user local $OutputBase/state/approvals.json, which only the local owner writes. The schema edits are description text only, and noise_allowlist is still accepted, so existing files keep validating.
  • Instruction-surface lens: the doc edits remove no guardrail. The remediation-policy.md abort condition is now stricter and matches the code.
  • Manifests: no plugin.json or marketplace.json changes, so there are no manifest <field> lines to report.
  • Protected-class list: the instruction exception register was not consulted, since nothing in the diff deletes or softens a rule.

@github-actions

Copy link
Copy Markdown
Contributor

claude-review reviewed f95e4e66c6b575be3f1f53bbe38b1b77164cce96: 0 finding(s), 0 as inline comments.

No findings on #9260 at f95e4e6. I read the diff and the PR context, and checked the call path in the source. I ran no tests and no PowerShell, so the Pester results in the PR body are the author's claim and I haven't confirmed them.

What I checked

  • Skip logic. Restart-StoppedService.ps1 skips an excluded name (-in, case-insensitive, exact match) before any Get-Service or Start-Service call, and it adds no attempt record.
  • Orchestrator wiring. Invoke-Remediation now splats ExtraArgs and captures -InformationVariable. Both remediation scripts, Restart-StoppedService.ps1 and Clear-TempFiles.ps1, declare [CmdletBinding()], so the common parameter binds for both. With no information records the variable is an empty list, so no blank remediation_info line is logged.
  • Get-ServiceExclusion. It handles a null state and a missing level at each step, and a single string value. The caller wraps the result in @(), so an empty result splats cleanly as an empty string[].
  • Hand-edited exclusions survive setup. Set-ApprovalState edits the loaded JSON document in place rather than rebuilding it, so a hand-edited check_overrides.services.service_exclusions is not dropped when setup writes the file.
  • Schema. It keeps noise_allowlist, so existing files still validate (the schema is additionalProperties: false).
  • Docs and changelog. The docs and the changelog fragment match the code. bump: patch fits a fix that wires a documented key.
  • Closing issue. The PR states "No related issue", so there are no acceptance criteria to check.
  • Manifests. The diff changes no plugin manifest, so none of the manifest checks apply.

Test coverage
The new tests cover the skip (excluded service not started, others started, partial name not matched, information-stream message) and Get-ServiceExclusion at each missing level. The PR body says no test drives the orchestrator's remediation dispatch. I traced that path by reading and found no defect.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 23, 2026
Merged via the queue into main with commit b3cc9fb Oct 23, 2026
49 of 59 checks passed
@kyle-sexton
kyle-sexton deleted the fix/machine-health-check-overrides branch October 23, 2026 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant