feat(kubevirt): add run policy support to VM lifecycle management

[awels]

Adds a new run_policy parameter to the vm_lifecycle tool that allows users to control the VM's runStrategy when starting a virtual machine.

The parameter supports three policies:

• HighAvailability: VM runs continuously (sets runStrategy to Always)
• RestartOnFailure: VM restarts on failure (sets runStrategy to RerunOnFailure)
• Once: VM runs once and stops after completion (sets runStrategy to Once)

The run_policy parameter is optional and defaults to HighAvailability to maintain backward compatibility with existing usage.

Changes include:

• Updated StartVM function to accept RunPolicy parameter
• Added 19 unit tests covering all run policy combinations
• Added 3 integration tests for vm_lifecycle tool
• Updated tool schema with enum values and documentation
• Auto-generated README.md updates

[awels]

/cc @lyarwood

[ksimon1]

/lgtm

[Cali0707]

/run-mcpchecker kubevirt

[lyarwood]

The run_policy value is cast directly to RunPolicy without server-side validation. getRunStrategyFromRunPolicy silently defaults invalid values to Always, which masks errors. Consider adding explicit validation:

if !kubevirt.IsValidRunPolicy(runPolicy) {
    return api.NewToolCallResult("", fmt.Errorf("invalid run_policy %q: must be one of HighAvailability, RestartOnFailure, Once", runPolicyStr)), nil
}

[awels]

Done

[lyarwood]

nit: run_policy is parsed for all actions but only used by start. Consider moving this inside the ActionStart case to avoid confusion.

[awels]

Done

[Cali0707]

/run-mcpchecker kubevirt

[Cali0707]

@awels can you fix the merge conflict on the readme?

[awels]

Sure give me a bit to rebase.

[awels]

@Cali0707 I rebased if you could run the tests?

[manusa]

Overview

Independent review of the diff (not the PR description). The feature is well-scoped and the public-API change is backward-compatible for run_policy, but there are several issues worth addressing before merge — most notably a latent footgun in getRunStrategyFromRunPolicy, a stale README.md that does not match the new action description, and broken markdown rendering for the run_policy description.

Strengths

• Backward compatible: run_policy is optional and defaults to HighAvailability, which preserves the prior Always behavior for start/restart.
• Schema-side enum plus runtime validation gives the LLM an authoritative list.
• Good test coverage of policy/strategy combinations (pkg/kubevirt/vm_test.go, pkg/mcp/kubevirt_test.go).
• pkg/mcp/testdata/toolsets-kubevirt-tools.json is updated to match the schema.
• Migration to api.WrapParams in pkg/toolsets/kubevirt/vm/lifecycle/tool.go:77-84 matches the newer pattern used elsewhere in the codebase.

Issues

Critical (Must Fix)

• getRunStrategyFromRunPolicy silently falls back to RunStrategyAlways for unknown policies (pkg/kubevirt/vm.go:116-126). Today, all production callers gate on IsValidRunPolicy first, so the fallback is unreachable — but if any future caller forgets, an unknown policy boots the VM with Always instead of failing loudly. TestGetRunStrategyFromRunPolicy even encodes this surprise as expected behavior (pkg/kubevirt/vm_test.go:469-473, "Invalid policy defaults to Always"), making accidental regressions silent. Either drop the fallback and panic / return a second bool, error, or remove the dead branch entirely now that validation is mandatory.

Important (Should Fix)

• README.md action description is out of sync. pkg/toolsets/kubevirt/vm/lifecycle/tool.go:44 was shortened to "The lifecycle action to perform: 'start', 'stop', or 'restart'" (and pkg/mcp/testdata/toolsets-kubevirt-tools.json:168 was regenerated), but README.md:535 still shows the old 'start' (changes runStrategy to Always), 'stop' (changes runStrategy to Halted), or 'restart' (stops then starts the VM). Running make update-readme-tools produces a non-empty diff, so the auto-generated section was not regenerated.

• run_policy description renders as broken markdown in README.md. The description in pkg/toolsets/kubevirt/vm/lifecycle/tool.go:53-57 embeds \n - 'HighAvailability': … etc. The README generator (internal/tools/update-readme/main.go:90-97) emits each property at 2-space indent, so the embedded - bullets land at the same indent level as the run_policy line itself — they render as siblings of run_policy, not as nested explanations. README.md:539-541 show three "Once / HighAvailability / RestartOnFailure" bullets that look like additional top-level parameters, and README.md:542 (Defaults to 'HighAvailability' …) drops out of the list entirely because it has no indent. Move the option list into the parent action/run_policy description as a single line, or render the options outside the schema description, or deeper-indent the bullets (e.g. \n - …).

• Misleading "started successfully" message when only the strategy changes (pkg/toolsets/kubevirt/vm/lifecycle/tool.go:104-108, pkg/kubevirt/vm.go:73-74). StartVM now returns wasStarted == true whenever currentStrategy != desiredStrategy, which includes transitions where the VM was already running (e.g. Always → Once). The tool then says # VirtualMachine started successfully with run policy 'Once', even though the VM never stopped. Either differentiate the path in StartVM (e.g. return a tri-state started | strategyChanged | noChange) or branch the message on whether currentStrategy == RunStrategyHalted / empty. The doc comment on StartVM overloads "started" with "strategy changed" in the same way and should be updated alongside.

• TestGetRunStrategyFromRunPolicy violates the project's black-box testing rule (pkg/kubevirt/vm_test.go:448-484). CLAUDE.md is explicit: tests should be black-box and not access unexported functions; getRunStrategyFromRunPolicy is lowercase. The mapping is already fully covered by TestStartVM (via wantRunStrategy) and TestRestartVMWithDifferentRunPolicies. Delete this test — keeping it also locks the buggy Always fallback in as expected behavior (see Critical above).

• run_policy is validated twice. pkg/toolsets/kubevirt/vm/lifecycle/tool.go:97-99 and :125-127 validate, then pkg/kubevirt/vm.go:77-79 and :196-198 validate again — same check, four hard-coded copies of the error string. Pick one layer (the library is the public API, so prefer keeping the check there) and drop the other. As-is, adding a fourth policy means touching four strings.

Minor (Nice to Have)

• Unused exported constants (pkg/kubevirt/vm.go:19,22). RunStrategyWaitAsReceiver has zero references anywhere; RunStrategyManual is referenced only from tests in the same package. Both inflate the exported surface for downstream consumers without a current need. Either remove them or introduce them when a production caller actually needs them. RunStrategyRerunOnFailure / RunStrategyOnce are legitimately used.

• Type declaration style (pkg/kubevirt/vm.go:13-14). Two stacked type X string lines are valid Go but a grouped type ( … ) block reads better when introducing a sibling type. Optional polish.

• Stale comment (pkg/kubevirt/vm.go:92, // Check if already running). With the new semantics this branch checks "already at the desired strategy", not "already running". Update the comment so it doesn't describe the old behavior.

• Action description loses useful detail. Trimming action to just the three verbs (pkg/toolsets/kubevirt/vm/lifecycle/tool.go:44) removes the runStrategy mapping that an LLM previously relied on to predict behavior. If the goal was to move that mapping into the run_policy description, consider keeping a short "(see run_policy for details)" hint on action so the link is discoverable.

Assessment: Ready to merge? No — with fixes

The README desync and the broken-markdown rendering should be fixed before merge (auto-generated docs are easy to drift again later), and the silent Always fallback in getRunStrategyFromRunPolicy is worth tightening up while the file is open. The remaining items are quality-of-life.