feat(wfctl): validate iacProvider.computePlanVersion in plugin manifest (#693)#696
Merged
Conversation
…st (#693) Phase 2.1 follow-up to workflow#640 Phase 2 cascade (PR #694, v0.54.0). Per ADR 0024 + ADR 0040 hard-cutover discipline + Phase 1 Assumption 8: findIaCPluginDir previously json.Unmarshal'd the matching plugin manifest and returned the raw iacProvider.computePlanVersion string without validation. A typo (V2, v2.0, two, v3) would silently route through the v1 dispatch path via wfctlhelpers.DispatchVersionFor's empty/unknown default — breaking the Phase 2 hard-cutover contract. Validation gate now hard-fails on the matching plugin manifest when computePlanVersion is not in {"", "v1", "v2"}. Operators see the misconfiguration loudly with an actionable error instead of silent fallback. Empty/missing field still permitted (defaults to v1 dispatch). Per workflow#693 issue body's 3-option design space, picked option 3 (lightweight enum check, minimum viable) over option 1 (full pluginmanifest package, ~200 LOC overkill) and option 2 (reuse existing schema/ JSON Schema validator, ~50 LOC). YAGNI applies — the field's domain is a 3-element enum closed by the proto spec. 7 test cases (table-driven): empty, v1, v2 pass; V2 (uppercase typo), v2.0 (decimal typo), two (word typo), v3 (future Phase 2.3 tag pre-introduction) all reject with the actionable error. Closes #693.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a lightweight validation gate for IaC plugin manifest iacProvider.computePlanVersion so wfctl rejects invalid dispatch-version values instead of accepting silent v1 fallback behavior.
Changes:
- Validates matching IaC plugin manifests against
"","v1", and"v2". - Adds table-driven tests for accepted and rejected manifest values.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cmd/wfctl/deploy_providers.go |
Adds compute plan version enum validation during IaC plugin discovery. |
cmd/wfctl/deploy_providers_compute_plan_version_test.go |
Adds regression tests for valid and invalid manifest values. |
Comments suppressed due to low confidence (1)
cmd/wfctl/deploy_providers.go:164
- After adding this enum check, the function comment above still says the returned raw string is unconstrained and that this loader performs only minimal
json.Unmarshalvalidation. That documentation is now stale and will mislead future callers about whether invalid values can be returned from this path.
switch m.IaCProvider.ComputePlanVersion {
case "", "v1", "v2":
// valid
Comment on lines
+162
to
+166
| switch m.IaCProvider.ComputePlanVersion { | ||
| case "", "v1", "v2": | ||
| // valid | ||
| default: | ||
| return "", "", false, fmt.Errorf( |
| t.Fatalf("write manifest: %v", writeErr) | ||
| } | ||
|
|
||
| name, gotVer, _, err := findIaCPluginDir(pluginDir, "test-provider") |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Contributor
Author
Copilot review acknowledgementCopilot raised that the validated The PR body's framing slightly overstated the validation's role in dispatch — the gRPC capability is runtime-authoritative for dispatch routing. The plugin.json field is metadata for registry/install-time tooling. The validation gate is still valuable as defense-in-depth:
Adding a 2nd test exercising end-to-end through Defensive gate stays; framing clarified above. CI re-run for the unrelated TestPluginConformanceArtifactNoGoModFailsCleanly flake in progress (test passes locally; flake not caused by this PR's changes). |
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.
Summary
Phase 2.1 follow-up to workflow#640 Phase 2 cascade (PR #694, v0.54.0). Adds the manifest validation gate at
cmd/wfctl/deploy_providers.go::findIaCPluginDirthat was flagged as missing in the Phase 1 inventory's Assumption 8.Without this gate, a typo in
iacProvider.computePlanVersion(V2, v2.0, two, v3) silently routes through the v1 dispatch path viawfctlhelpers.DispatchVersionFor's empty/unknown default — breaking the Phase 2 hard-cutover contract per ADR 0024 + ADR 0040.Design choice
Issue #693 listed 3 options:
pluginmanifestpackage (schema-driven, generic) — ~200 LOC, overkillschema/JSON Schema validator — ~50 LOC, more machinery than warrantedcomputePlanVersion ∈ {v1, v2}enum check — minimum viable, pickedPicked option 3 per YAGNI. The field's domain is a 3-element enum (
"","v1","v2") closed by the gRPC spec; a switch statement is the right tool.Behavior
DispatchVersionFor)"v1"/"v2"→ permittedplugin %q manifest has invalid iacProvider.computePlanVersion %q (must be "", "v1", or "v2")Validation fires only on the matching plugin manifest (not on every plugin in the directory) — so unrelated misconfigured plugins don't block resolving the right one.
Test plan
TestFindIaCPluginDir_ComputePlanVersionValidation: empty, v1, v2 pass; V2 (uppercase typo), v2.0 (decimal typo), two (word typo), v3 (future Phase 2.3 tag pre-introduction) all reject with the actionable errorGOWORK=off go test ./cmd/wfctl/ -run 'TestFindIaCPluginDir_ComputePlanVersionValidation' -vRollback
Revert this commit. Pre-existing v1 behavior restored (silent fallback on typo).
Closes #693.