docs: plugin-modules-on-iac plan + ADR 0038 (Configure-RPC sibling for module factories)#682
Merged
Merged
Conversation
…ories Closes the architectural gap surfaced mid-execution of the locked B/C/D plan: plugins served via sdk.ServeIaCPlugin currently can't register ModuleFactories/StepFactories (the iacPluginServiceBridge implements only GetContractRegistry + GetManifest; everything else returns Unimplemented). This blocks B/C/D's standalone-modules extraction (Tasks 8/9/23 + downstream Tasks 14-18, 27-29). Approach: extend IaCServeOptions with Modules + Steps factory maps; iacPluginServiceBridge delegates the Module/Step lifecycle RPCs to the existing grpc_server.go PluginService implementation (extracted into a shared helper so legacy sdk.Serve and the new bridge use one source of truth for handle state). Backwards compatible; one entrypoint preserved; no proto change. Absorbs the locked B/C/D plan's blocked tasks so the plan needs no unlock. ADR 0038 records the decision (Approaches A/B/C, why A wins, the load- bearing assumption about grpc_server.go extractability). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Critical: corrected iacserver.go citation (line 196 → struct at 206-218, forward-extension comment at 200-205). Important: switched IaCServeOptions.Modules/.Steps to sdk.ModuleProvider / sdk.StepProvider (eliminates the factory-map-to-PluginProvider adapter shim — same interface grpc_server.go already consumes). Added explicit non-goals for MessageAwareModule (no broker plumbed through iacGRPCPlugin) and TypedModuleProvider (STRICT_PROTO contracts) for v1 scope. Rephrased the misleading "no structpb" claim to clarify no NEW structpb surface is introduced (Config remains structpb on the legacy path). Minor: PR-count estimate now a range (5+/15-20); plugin.json ↔ runtime parity test pinned with the locked-plan precedent; user-direction citation added to §Problem. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Important findings addressed by precision: explicitly named the thin mapBackedProvider adapter (not "no shim" — the adapter IS the shim, just small ~30 LOC) so plan-writing isn't surprised by the constructor mismatch with newGRPCServer(PluginProvider). Added explicit nil-broker test note (SDK extension task includes a regression-guard test that SetMessagePublisher/SetMessageSubscriber are never called via the bridge path). Added explicit minEngineVersion floor note in Migration (mirrors the locked B/C/D plan's universal rule). Cycle 2 reviewer verdict: "design is substantially correct and implementable" — no architecture changes needed; documentation precision fixes only. PASS-with-acceptance per the recorded findings + recommended fixes incorporated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements the SDK extension from decisions/0038 + absorbs the locked B/C/D plan's blocked scope (aws/gcp standalone modules, Phase B + Phase C workflow-core deletions). Builds on the design's mapBackedProvider adapter approach + the locked plan's already-shipped IaCStateBackend contract + Configure RPC. Plan structure: PR 1 (workflow SDK extension) → PR 2/3 (aws/gcp plugin modules + releases, parallel after workflow v0.53.0 tag) → PR 4 (Phase B core deletion, deps PR 2 + locked-plan PR 5) → PR 5 (Phase C core deletion, deps PR 3 + locked-plan PR 9). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Critical: Manifest() method added to mapBackedProvider (was missing, deterministic compile error); PR 3 pre-step added to coordinate the in-progress locked-plan Task #22 owner (otherwise commit-collision risk). Important: explicit subprocess runtime-launch validation steps in Tasks 7+11 (in-process bufconn was insufficient for the change class); explicit 'gh release view' pre-merge gate for PR 4 (#118 release tag); ContractRegistry() removed from mapBackedProvider (dead code path). Minor: credref.Reset() test-only helper + t.Cleanup pattern documented; Task 2 wording committed to bufconn-is-canonical with rationale. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Important: clarify CredInput.Source population path-routing — both the standalone-module and IaC-provider paths read config['credentials']['type'] from the raw YAML config (CloudCredentials.Extra never crosses the plugin boundary). Task 13's marker is consumed only by in-core paths PR 4 deletes. Two cycle-2 Minor findings (test-skeleton file reference + git add -A convention vs feedback memory) accepted as documentation-precision-only; the implementer/reviewer pipeline will catch the test-skeleton wording at implementation time, and the git status verify-step in the plan's commit blocks mitigates the scope-bleed risk the workspace memory addresses. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a locked implementation plan plus an accepted ADR describing how to extend sdk.ServeIaCPlugin so IaC plugins can also serve module + step factories by delegating those RPCs to the existing grpc_server.go implementation.
Changes:
- Added a scope-locked implementation plan for “Plugin Modules on IaC” (5 PRs / 19 tasks).
- Added a design document capturing the architecture and tradeoffs for the SDK extension.
- Added ADR 0038 documenting the decision to extend the IaC serve bridge (Approach A).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| docs/plans/2026-05-15-plugin-modules-on-iac.md.scope-lock | Adds the scope-lock hash for the new plan. |
| docs/plans/2026-05-15-plugin-modules-on-iac.md | Adds the implementation plan/task breakdown for SDK + plugin + core-deletion work. |
| docs/plans/2026-05-15-plugin-modules-on-iac-design.md | Adds the design doc for bridging module/step RPCs through the IaC serve path. |
| decisions/0038-plugin-modules-on-iac-serve-bridge.md | Adds ADR 0038 capturing the chosen approach and scope limits. |
Comments suppressed due to low confidence (2)
docs/plans/2026-05-15-plugin-modules-on-iac-design.md:117
- The IaCServeOptions comment says “no adapter shim sits between the IaC bridge and the existing handle-state / lifecycle code”, but later in this same design the implementation explicitly introduces a
mapBackedProvideradapter/shim (~30 LOC). Please reconcile this wording so the document is internally consistent (e.g., acknowledge the thin adapter while emphasizing there’s no parallel factory shape or lifecycle reimplementation).
// The map's value type is sdk.ModuleProvider — the SAME interface
// grpc_server.go's legacy sdk.Serve path already consumes — so no
// adapter shim sits between the IaC bridge and the existing handle-
// state / lifecycle code. Plugin authors wrap their factory functions
// in a thin sdk.ModuleProvider struct (the same wrapper non-IaC
// plugins already use); the SDK does not introduce a parallel
// factory-map shape.
docs/plans/2026-05-15-plugin-modules-on-iac-design.md:128
- This design describes adding an embedded
*moduleStepDelegate, but the later “Implementation mechanism” section (and the implementation plan) describe delegating directly to*grpcServercreated bynewGRPCServer(mapBackedProvider). Consider renaming/removingmoduleStepDelegatehere to match the intended concrete delegate type so implementers don’t look for a non-existent abstraction.
`iacPluginServiceBridge` gains an embedded `*moduleStepDelegate` (a small wrapper around `grpc_server.go`'s existing PluginService Module/Step implementation). When the delegate is wired, the bridge's `GetModuleTypes` / `CreateModule` / `InitModule` / `StartModule` / `StopModule` / `DestroyModule` / `GetStepTypes` / `CreateStep` / `ExecuteStep` / `DestroyStep` methods forward to it; when it is nil (zero-value `IaCServeOptions`), they fall through to `pb.UnimplementedPluginServiceServer`.
| # Plugin Modules on IaC — extending `ServeIaCPlugin` to also serve module + step factories | ||
|
|
||
| **Date:** 2026-05-15 | ||
| **Status:** Design — pre adversarial-design-review |
Contributor
Author
There was a problem hiding this comment.
Fixed in 07c0e05 (status line updated to reflect cycle-2 PASS-with-acceptance + locked timestamp).
|
|
||
| ## Decision | ||
|
|
||
| **Adopt Approach A.** Add `Modules map[string]sdk.ModuleProvider` + `Steps map[string]sdk.StepProvider` to `IaCServeOptions`. `ServeIaCPlugin` constructs a "hybrid" `pb.PluginServiceServer` whose `GetContractRegistry`/`GetManifest` come from the existing IaC bridge logic, and whose `GetModuleTypes`/`CreateModule`/`InitModule`/`StartModule`/`StopModule`/`DestroyModule`/`GetStepTypes`/`CreateStep`/`ExecuteStep`/`DestroyStep` come from the **existing** `plugin/external/sdk/grpc_server.go` PluginService implementation, parameterized over the supplied provider maps. The map value types are the same `sdk.ModuleProvider` / `sdk.StepProvider` interfaces non-IaC plugins already implement — no parallel factory shape, no adapter shim between the bridge and the existing handle-state code. Single registered `pb.PluginServiceServer`; no double-registration; no proto change; one entrypoint. Backwards compatible (zero-value options = current behavior). Approach (B) rejected — adds a third entrypoint and partially undoes the cutover's UX consolidation. Approach (C) rejected — re-creates the registration-omission bug class the cutover deleted. |
Contributor
Author
There was a problem hiding this comment.
Fixed in 07c0e05 (ADR Decision section now explicitly names the thin mapBackedProvider adapter as a shim, while preserving the key win: single source of truth via grpc_server.go's existing impl).
|
|
||
| ```go | ||
| func TestIaCBridge_ModulesAndSteps_Delegate(t *testing.T) { | ||
| // Construct a mapBackedProvider via NewIaCServeOptions |
Contributor
Author
There was a problem hiding this comment.
Acknowledged — cosmetic comment in a Task-1 code skeleton. The snippet itself constructs IaCServeOptions{...} directly (which is what implementer-1 did at 9756c29), zero functional impact. Plan is scope-locked; lock hook blocks plan edits. Filing as docs-precision follow-up; not blocking.
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)
|
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
docs/plans/2026-05-15-plugin-modules-on-iac.md(5 PRs / 19 tasks). Extendssdk.ServeIaCPluginto also serve module + step factories via a thinmapBackedProvideradapter; absorbs the locked B/C/D plan's blocked tasks (aws/gcp standalone modules + workflow-core deletions).decisions/0038— records the SDK-extension decision (Approach A:IaCServeOptions.Modules/.Steps+mapBackedProviderreusinggrpc_server.go's existing PluginService impl).Docs/decisions only. Lands plan + ADR on
mainso the 5 execution PRs (each offmain) can referencedecisions/0038+ the plan tasks.Test plan
plan-scope-check.shPASS (manifest 5 PRs / 19 tasks)🤖 Generated with Claude Code