fix(plugin/external): engine disk-manifest fallback + SDK embed helper + STRICT_PROTO _-prefix strip#643
Merged
Merged
Conversation
…_-strip) Two upstream bugs surfaced by BMW v0.51.2 smoke gate: 1. IaC plugins via sdk.ServeIaCPlugin lack GetManifest impl → engine synthesized manifest has empty Version → Validate() rejects → registration fails. 2. Engine-injected _config_dir contaminates STRICT_PROTO module configs → protojson DiscardUnknown=false rejects unknown field → module init fails. Fixes (workflow v0.51.3): 1. sdk.EmbedManifest helper + ManifestProvider option on all Serve* helpers. Plugins use //go:embed plugin.json to declare manifest at compile time. 2. Engine strips _-prefix keys from cfg before STRICT_PROTO encoding in createTypedConfigRequest. _-prefix reserved for engine internals. ADR-0031 records the design + alternatives + trade-offs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…2 Minor) R1 Critical findings: - C1: EmbedManifest used encoding/json into *pb.Manifest; mismatches camelCase plugin.json field names vs snake_case proto JSON tags. Fix: parse via the canonical *plugin.PluginManifest type which already has correct tags. - C2: ServeIaCPlugin uses iacPluginServiceBridge for PluginService, NOT grpcServer. GetManifest handler lives on bridge. Fix: wire ManifestProvider into IaCServeOptions + bridge.GetManifest override. R1 reviewer Option 3 adopted (significant pivot): - manager.go:108 already loads disk plugin.json via pluginpkg.LoadManifest. - Bug 1 PRIMARY fix moves engine-side: thread disk manifest into adapter as fallback when gRPC GetManifest returns empty/Unimplemented. ZERO plugin-side change required to unblock BMW. - SDK helper (EmbedManifest) becomes forward-looking opt-in for plugins that want compile-time embedded manifest (defense-in-depth, not strictly required). R1 Important findings addressed: - A1 reframing: BMW unblocks the moment v0.51.3 ships; DO v1.0.12 is optional. - stripInternalKeys clarified as copy-on-clean (fresh map, not in-place). - ManifestProvider interface removed per YAGNI; concrete *plugin.PluginManifest is the type. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…stFromDisk field map
…urce overlay + F3 grep + F4 Validate semantics + F5 step path + F6 panic warning
…valid-shape tests + R2-3/4 task 6 clarity + R2-5 appendix + R2-6 launch step
…s green; release smoke pending)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR delivers three targeted fixes to the external plugin system to unblock strict-cutover IaC plugins and STRICT_PROTO plugins: (1) engine-side fallback to the on-disk plugin.json manifest when gRPC GetManifest is unimplemented/empty, (2) an SDK helper + options to embed and serve a canonical manifest via gRPC, and (3) stripping engine-internal _-prefixed config keys (e.g. _config_dir) before STRICT_PROTO typed config encoding.
Changes:
- Thread the manager-loaded
*plugin.PluginManifestintoNewExternalPluginAdapterand use it as the authoritative fallback when gRPC manifest isUnimplementedor has emptyVersion. - Add
sdk.EmbedManifest/sdk.MustEmbedManifestandsdk.WithManifestProvider(plus IaC bridge support viaIaCServeOptions.ManifestProvider) so plugins can compile-time embedplugin.jsonand serve it viaGetManifest. - Strip
_-prefixed internal keys beforemapToTypedAnyto prevent STRICT_PROTO protojson decode failures while preserving the legacy*structpb.Structpath.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin/external/sdk/serve.go | Adds ServeOption and WithManifestProvider; makes Serve variadic to apply options. |
| plugin/external/sdk/serve_full.go | Threads ServeOption through ServePluginFull and forwards to Serve. |
| plugin/external/sdk/manifest.go | Adds EmbedManifest/MustEmbedManifest to parse+validate embedded plugin.json into canonical *plugin.PluginManifest. |
| plugin/external/sdk/manifest_test.go | Unit tests for EmbedManifest and MustEmbedManifest (happy path + validation/error cases). |
| plugin/external/sdk/iacserver.go | Threads IaCServeOptions.ManifestProvider into the IaC PluginService bridge and adds bridge GetManifest. |
| plugin/external/sdk/iacserver_internal_test.go | Internal-package tests for IaC bridge GetManifest behavior. |
| plugin/external/sdk/grpc_server.go | Adds diskManifest override and makes GetManifest prefer it over provider.Manifest(). |
| plugin/external/sdk/grpc_server_test.go | Tests manifest precedence (disk override vs provider fallback). |
| plugin/external/manager.go | Passes the manager-loaded manifest into the adapter constructor. |
| plugin/external/integration_test.go | In-process e2e test ensuring manager-loaded disk manifest is used when gRPC GetManifest is unimplemented. |
| plugin/external/convert.go | Adds stripInternalKeys helper to remove _-prefixed internal keys from typed-encode input. |
| plugin/external/convert_test.go | Unit tests for stripInternalKeys + copy-on-clean behavior. |
| plugin/external/adapter.go | Adds manifestFromDisk, updates adapter constructor signature/behavior, and strips internal keys before STRICT_PROTO typed encoding. |
| plugin/external/adapter_test.go | Updates constructor call sites and adds regression tests for disk-manifest fallback/overlay and _config_dir stripping. |
| cmd/wfctl/plugin_conformance.go | Updates conformance path to call new adapter signature (passing nil disk manifest). |
| docs/plans/2026-05-12-strict-contracts-ergonomics.md.scope-lock | Adds scope-lock hash artifact. |
| docs/plans/2026-05-12-strict-contracts-ergonomics.md | Adds implementation plan documenting the fixes, tests, and rollout steps. |
| docs/plans/2026-05-12-strict-contracts-ergonomics-design.md | Adds the design document motivating/justifying the three fixes. |
| docs/audit/2026-05-12-v0.51.3-smoke-transcript.txt | Adds runtime-launch-validation transcript and local verification notes. |
| docs/audit/2026-05-12-underscore-prefix-audit.md | Adds audit showing no _-prefixed proto fields in scoped repos. |
| decisions/0031-strict-contracts-ergonomics.md | Adds ADR capturing the rationale/decision and consequences. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
- Plan tech-stack line: Go 1.23 → 1.26.0 (matches go.mod) - TestIaCBridgeGetManifestUsesProvider: use Validate-passing manifest fixture and assert Name/Version/Author/Description so any of the four mapped fields regressing fails the test
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
Three surgical fixes for workflow v0.51.3 that unblock BMW deploy + every strict-cutover IaC + STRICT_PROTO plugin:
Engine-side disk-manifest fallback (Bug 1 PRIMARY fix — zero plugin change).
manager.go:108already loads*plugin.PluginManifestviapluginpkg.LoadManifest+Validate. Thread that intoNewExternalPluginAdapteras a fallback when gRPCGetManifestreturnscodes.Unimplemented(strict-cutover IaC plugins served viasdk.ServeIaCPlugin) or an empty Version. DO v1.0.11 registers cleanly via this fallback without re-release.SDK
EmbedManifesthelper +WithManifestProvideroption (Bug 1 FORWARD-LOOKING). Plugin authors//go:embed plugin.json+ callsdk.MustEmbedManifest(json), pass viasdk.WithManifestProviderorIaCServeOptions.ManifestProvider. Parses via canonical*plugin.PluginManifest(camelCase JSON tags), NOT*pb.Manifest(snake_case). Engine-fallback path is the immediate unblock; helper is opt-in defense-in-depth + portability.Engine
_-prefix strip increateTypedConfigRequest(Bug 2). Strips_config_dir(and other engine internals) fromcfgbeforemapToTypedAnyfor STRICT_PROTO modules whose protojsonDiscardUnknown: falserejects unknown fields. Copy-on-clean — legacy*structpb.Structpath retains_config_dirfor legacy modules.Design
See:
docs/plans/2026-05-12-strict-contracts-ergonomics-design.md(R2 PASS via 2 adversarial review cycles).Implementation Plan
See:
docs/plans/2026-05-12-strict-contracts-ergonomics.md(R2 PASS via 2 adversarial review cycles + alignment PASS + scope-locked at sha256 4c9a451758fd).ADR
See:
decisions/0031-strict-contracts-ergonomics.md.Scope Manifest
_-prefix stripChanges (8 production commits)
manifestFromDiskhelper +NewExternalPluginAdapter3-arg signature; threaded frommanager.go:169+cmd/wfctl/plugin_conformance.go:327. 3 new tests + 18 updated call sites.TestEngineManifestValidatesAfterDiskOverlay+ precedence-rule comment inadapter.go(single source of truth = Task 1 constructor overlay).stripInternalKeyshelper + wire intocreateTypedConfigRequest. 8 new tests including module + step + legacy + DoesNotMutateInput.EmbedManifest+MustEmbedManifest. 10 tests covering happy path + 9 rejection paths (missing Name/Version/Author/Description, malformed JSON, empty bytes, invalid-semver, invalid-name-shape, panic).WithManifestProviderintogrpcServer.GetManifest. VariadicServe(...ServeOption)+ServePluginFull(...ServeOption)preserves existing callers.IaCServeOptions.ManifestProvider+iacPluginServiceBridge.GetManifestoverride. ExtractregisterIaCServicesOnlypreserving all nil + typed-nil hardening. New internal-test file for unexported bridge access._-prefix proto fields across workflow engine + 6 wave plugins (66 .proto files). Verdict: PASS — zero collisions.TestManagerLoadPluginThreadsDiskManifestToAdapter+ runtime-launch-validation transcript with pre-tag ritual (v0.51.3-rc1 → DO plugin rebuild → subprocess smoke → promote-or-rollback).Test Plan
GOWORK=off go test ./plugin/external/ ./plugin/external/sdk/→ all PASS (local)go build ./...→ exits 0 (verified per task)go build ./cmd/wfctl→ exits 0 (F1 conformance caller correctness)TestStripInternalKeysDoesNotMutateInputpb.Manifestscalar fields mapped inmanifestFromDiskServe(p)+ 3-argServePluginFull(p, cli, hooks)+ zero-valueIaCServeOptions{}still compile via variadic / optional fieldregisterIaCServicesOnlyretains nil-server, nil-provider, typed-nil-reflect checks verbatimdocs/audit/2026-05-12-v0.51.3-smoke-transcript.txt. Tagv0.51.3-rc1→ rebuild DO plugin against rc1 SDK → stage → launchcmd/serveragainst staged DO binary → verifyplugin "digitalocean" loaded successfullylog line + novalidate manifesterrors + noproto: unknown field "_config_dir"errors → promote rc1 → v0.51.3.Review process
Pre-existing test environment notes (NOT regressions)
cmd/wfctl TestInfraMultiEnv_E2E— fails onorigin/mainwith identical wording (no plugin found for IaC provider "digitalocean" in ./data/plugins); env-dependent.example.test/wfctl-ci/pkg TestFallbackRuns— synthetic negative-test fixture injected atcmd/wfctl/ci_run_test.go:178; outer testTestRunCIRunTestFallsBackToGoTestWhenNoConfiguredTestsPASSES — inner FAIL line is the test's success signal.Both reproduce on
origin/main; not v0.51.3 regressions.🤖 Generated with Claude Code