Skip to content

feat(machine-health): let machine-health:setup add and remove service exclusions - #10524

Merged
melodic-automation-lanes[bot] merged 1 commit into
mainfrom
work-resolve-issue/issue-9258-40572861348-1
Oct 27, 2026
Merged

melodic-automation-lanes[bot] merged 1 commit into
mainfrom
work-resolve-issue/issue-9258-40572861348-1

Conversation

@melodic-automation-lanes

Copy link
Copy Markdown
Contributor

Closes #9258

Summary

Opened by the work-resolve-issue lane. The run's own summary:

Set-ApprovalState gains exclude-service and include-service actions (with -ServiceName) that write approvals.json check_overrides.services.service_exclusions under approvals.lock, schema-checked and replaced through Write-StateFile like approve and revoke. /machine-health:setup apply exposes them as exclude-service=<name> and include-service=<name>, and the schema and approvals.md no longer call the key hand-edited.

Not verified

  • Pester tests (Set-ApprovalState.Tests.ps1, SkillCommands.Tests.ps1) were not run: this lane has no pwsh and runs only the named wrappers. They are left to the PR's Windows CI. The reviewer checked by reading that SkillCommands still sees 10 PowerShell blocks with the apply template unchanged
  • PSScriptAnalyzer lint of Set-ApprovalState.ps1 and its tests was not run (no pwsh in this lane); left to the PR's CI
  • new-changelog-fragment refused (detached HEAD, no branch name), so the fragment was written by hand following the existing naming pattern; check-changelog-fragments accepts it
  • A service name containing an apostrophe or typographic single quote cannot be passed through setup apply (the skill refuses it so it never reaches the command line); such names are rare and Set-ApprovalState itself still accepts them when called directly
  • Untracked root files (.bash_profile, .bashrc, .gitconfig, .idea, .mcp.json, .profile, .ripgreprc, .vscode, .zprofile, .zshrc) appear in git status; this run did not create them (runner environment) and left them alone

Fix

  • plugins/machine-health/skills/audit/scripts/windows/lib/Set-ApprovalState.ps1: new exclude-service / include-service actions and -ServiceName parameter. Exclude appends the name, creating check_overrides/services/service_exclusions when absent, and skips a name already listed in any case (Changed fa
  • plugins/machine-health/skills/audit/tests/windows/lib/Set-ApprovalState.Tests.ps1: new 'exclude-service and include-service' context. It covers create from empty, append keeping remediations/migration/noise_allowlist, duplicate add in another case, case-insensitive remove, removing an absent name (a
  • plugins/machine-health/skills/setup/SKILL.md: documents exclude-service=<name> and include-service=<name> in the description, the argument-hint and Arguments line, action routing, the non-interactive write paths and check step 3. Adds the single-quote rule for passing a service name into
  • plugins/machine-health/skills/audit/catalog/schemas/approvals.schema.json: top-level description drops the 'except service_exclusions, which setup cannot write yet' exception; the service_exclusions description names the setup arguments that write it.
  • plugins/machine-health/skills/audit/reference/shared/approvals.md: the Writing section lists the new setup arguments and no longer implies an exempt key. Per-host check overrides drops the hand-edit sentence, names the arguments and states the name rule, citing CreateServiceW.
  • .changes/machine-health/feat-9258-setup-service-exclusions-5c2e8a17.md: minor-bump changelog fragment (Added and Changed).

Verification

The runner ran each check from the base copy against e9d171b:

  • scripts/check-changelog-fragments.sh --check: passed

The run reports:

  • read https: //learn.microsoft.com/en-us/windows/win32/api/winsvc/nf-winsvc-createservicew: lpServiceName is at most 256 characters, compared case-insensitively, and '/' and '' are not valid service name characters
  • .lane/checks/bin/check-changelog-fragments: all 10 fragments valid
  • .lane/checks/bin/check-affected: first run failed validate-plugins (setup argument-hint 105 characters, over the 100 budget). After shortening the hint: 11 passed, 7 skipped, 0 failed
  • .lane/verify/bin/cant-fail-scan --file Set-ApprovalState.Tests.ps1: 20 blocks parsed. The one finding (rule-weak-oracle, report-only) is the existing 'refuses a malformed key' test at line 150, not a new test
  • fresh-context review subagent over the whole diff: no correctness bugs. Five low findings, all fixed: SKILL quoting rationale, write-path header wording, an overstated approvals.md sentence, padded names accepted, and a non-array list giving a confusing result
  • The lane run: https://github.com/melodic-software/claude-code-plugins/actions/runs/40572861348

Left out: the items under Not verified.

Related

#9258

Lane-Activity: work-resolve-issue/resolve-issue
@github-actions

Copy link
Copy Markdown
Contributor

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

No security issues found in this pull request. It changes only machine-health plugin files, so no trusted-path glob matches. I read the full diff and the PR context but did not run any code.

critical-findings: 0

What I checked

  • Command injection through the service name. /machine-health:setup apply exclude-service=<name> puts the name into a PowerShell command in single quotes (plugins/machine-health/skills/setup/SKILL.md, lines 416-419 of the head file). Inside single quotes, $ and backticks stay literal, so only a quote character can break out. The skill refuses ', U+2018 to U+201B and line breaks before running anything, which covers the quote characters PowerShell treats as terminators.
    • That prose rule is the only thing stopping a quote that arrives on the command line, because Set-ApprovalState sees the name only after PowerShell has parsed it.
    • The name comes from the owner who types the argument. The skill sets disable-model-invocation: true, so only a person can invoke it. I found no path where an untrusted party supplies the name.
  • Input validation in Set-ApprovalState.ps1 (head lines 125-132 and 160-188). $assertServiceName rejects a blank name, one with leading or trailing whitespace, one over 256 characters, and any control character, / or \. It runs through Validate before the write, and the new tests cover each case. Names go into JSON as typed strings, so there is no injection into the JSON document.
  • Write path. The new actions reuse the existing approvals.lock, schema check and Write-StateFile atomic replace, which I read as unchanged. A service_exclusions value that isn't an array throws without writing. A null or non-string list entry makes GetValue[string]() throw, so it fails closed.
  • Effect of the exclusion list. It only stops Restart-StoppedService from starting a named service. It is per host and under the owner's control. It adds no privilege and gives no new reach.
  • Other files. The schema description, approvals.md, the changelog fragment and the tests are documentation or test changes. No manifest, hook, workflow, token or merge-hold file changed, so there are no manifest <field> lines.

Cost: none. The change only edits files in the repository, and the work-resolve-issue lane spends prepaid or subscription usage.

@github-actions

Copy link
Copy Markdown
Contributor

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

No findings. I read the whole diff, the PR context, the surrounding Set-ApprovalState.ps1, approvals.schema.json and the /machine-health:setup apply section of SKILL.md. I did not run any code, because this lane has no shell. The Pester tests and PSScriptAnalyzer run were not run here, and the PR says they were not run either. They are left to Windows CI.

What I checked

  • Key path matches the reader. The new actions write check_overrides.services.service_exclusions, the same path Get-ApprovalState.ps1 and Restart-StoppedService.ps1 read. The schema's additionalProperties under check_overrides accepts services as a key.
  • Name validation. It runs before the state directory, the lock or any file exists (Set-ApprovalState.ps1:216). The tests that expect no file created for a bad name are consistent with that.
  • Write path. The new actions reuse the existing lock, Write-StateFile replace and Test-Json schema check. A schema failure throws before any replace, and $false from Apply yields Changed = $false with no write.
  • Closing issue machine-health: let /machine-health:setup add and remove service exclusions #9258. Both criteria are met:
    • /machine-health:setup can add and remove an exclusion through Set-ApprovalState (validated, locked, atomic).
    • The schema description and approvals.md no longer call the key hand-edited.
    • The new tests cover the add, remove, validation, lock and schema paths.
  • Changelog fragment. The minor bump fits an added feature.

There is no plugin manifest change, so no manifest <field> lines apply. There is no billing or cost impact, so I returned no hold. The PR body has no embedded instructions to the reviewer.

Unverified claims. The PR's "Verification" and "Not verified" sections are the author's own claims. I did not confirm them.

@melodic-automation-lanes

Copy link
Copy Markdown
Contributor Author

Unattended merge: native auto-merge leaves this pull request unarmed at head e9d171b296608020b04d4924354bb7670a1c15c7. The lanes clear this reason on their own; the next scheduled run judges it again.

pr-merge's gate refuses its verifier verdict (verify:missing)

@melodic-automation-lanes

Copy link
Copy Markdown
Contributor Author

The unattended merge path armed native auto-merge at head e9d171b296608020b04d4924354bb7670a1c15c7.

@melodic-automation-lanes
melodic-automation-lanes Bot added this pull request to the merge queue Oct 27, 2026
Merged via the queue into main with commit 8232b58 Oct 27, 2026
42 checks passed
@melodic-automation-lanes
melodic-automation-lanes Bot deleted the work-resolve-issue/issue-9258-40572861348-1 branch October 27, 2026 21:07
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.

machine-health: let /machine-health:setup add and remove service exclusions

0 participants