Skip to content

refactor: remove PipelineHandler getter from pipelinesteps plugin#118

Merged
intel352 merged 5 commits into
mainfrom
refactor/issue-60-remove-plugin-handler-getter
Feb 23, 2026
Merged

refactor: remove PipelineHandler getter from pipelinesteps plugin#118
intel352 merged 5 commits into
mainfrom
refactor/issue-60-remove-plugin-handler-getter

Conversation

@intel352
Copy link
Copy Markdown
Contributor

Summary

  • Removes the PipelineHandler() getter from plugins/pipelinesteps/plugin.go — callers no longer reach inside the plugin to configure it
  • Plugin now implements SetStepRegistry() and SetLogger() optional interfaces, which the engine's LoadPlugin() detects and calls automatically
  • Plugin adds a WiringHook that injects dependencies into the handler and registers it as a named service (pipeline-workflow-handler)
  • cmd/server/main.go discovers the handler via service registry lookup when needed (e.g., for wiring SetEventRecorder post-start)
  • Engine's LoadPlugin() now supports stepRegistrySetter and slogLoggerSetter optional interfaces for all plugins

Closes #60

Test plan

  • go build ./... passes
  • go test ./... — all tests pass
  • golangci-lint run — 0 issues (verified by pre-push hook)

🤖 Generated with Claude Code

intel352 and others added 2 commits February 23, 2026 03:26
Move handler wiring from cmd/server/main.go into the plugin's own
lifecycle via optional setter interfaces and a wiring hook:

- Plugin implements SetStepRegistry() and SetLogger() — engine's
  LoadPlugin() detects and calls these after registering factories
- Plugin adds a WiringHook that injects dependencies into the
  PipelineWorkflowHandler and registers it as a named service
- cmd/server/main.go no longer reaches inside the plugin — it
  discovers the handler via service registry lookup when needed
  (e.g., to wire SetEventRecorder post-start)

Closes #60

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The plugin now configures itself internally via optional-interface injection
and a WiringHook rather than exposing a concrete handler reference to callers.

- engine.LoadPlugin() detects SetStepRegistry/SetLogger optional interfaces
  and injects the engine's step registry and slog.Logger into plugins after
  workflow handlers are registered
- Plugin.WiringHooks() wires those injected dependencies into the handler
  and registers it under PipelineHandlerServiceName in the app service registry
- cmd/server/main.go no longer retains a pipelinePlugin variable; EventRecorder
  wiring in registerPostStartServices looks up the handler via the service registry
- engine_pipeline_test.go finds the handler via type assertion instead of getter
- Removed pipelineEventSetter interface and pipelineHandler serviceComponents field

Closes #60

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings February 23, 2026 08:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the pipelinesteps plugin integration so callers no longer access a concrete handler via a plugin-specific getter, and instead rely on engine-driven optional interface injection and service registry discovery.

Changes:

  • Removed PipelineHandler() getter from plugins/pipelinesteps and added optional SetStepRegistry() / SetLogger() setters plus a wiring hook that registers the handler as a named service.
  • Extended StdEngine.LoadPlugin() to inject step registry and (optionally) a *slog.Logger into plugins that implement the new setter interfaces.
  • Updated server wiring and tests to discover the pipeline handler via the service registry rather than via direct plugin access.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
plugins/pipelinesteps/plugin.go Removes handler getter; adds optional setters + wiring hook; registers handler under a constant service name
engine.go Adds optional-interface injection for step registry and *slog.Logger during plugin load
cmd/server/main.go Removes direct plugin handler wiring; wires EventRecorder via service-registry lookup post-start
engine_pipeline_test.go Updates tests to locate the pipeline handler from the engine’s handler list
cmd/server/main_test.go Adjusts tests for updated buildEngine signature
plugins/http/plugin_test.go Formatting-only change
Comments suppressed due to low confidence (2)

plugins/pipelinesteps/plugin.go:145

  • app.RegisterService errors are currently ignored. If registration fails (e.g., name collision), the PipelineWorkflowHandler won’t be discoverable via the service registry and post-start wiring (SetEventRecorder) will silently not happen. Return the error (or at least log it) so mis-wiring fails fast and is diagnosable.
				// Register the handler as a service so callers can discover it
				// (e.g. to wire SetEventRecorder post-start) without a plugin-specific getter.
				_ = app.RegisterService(PipelineHandlerServiceName, p.pipelineHandler)
				return nil

cmd/server/main.go:871

  • If the pipeline handler service is missing from the registry or doesn’t implement SetEventRecorder, this branch becomes a silent no-op and pipeline execution events won’t be recorded without any signal. Consider adding a Debug/Warn log when the service isn’t found and when the type assertion fails, to make misconfiguration visible in production.
		if svc, ok := engine.GetApp().SvcRegistry()[pluginpipeline.PipelineHandlerServiceName]; ok {
			if ph, ok := svc.(eventRecorderSetter); ok {
				recorder := evstore.NewEventRecorderAdapter(app.stores.eventStore)
				ph.SetEventRecorder(recorder)
				logger.Info("Wired EventRecorder to PipelineWorkflowHandler")
			}
		}

Copilot AI review requested due to automatic review settings February 23, 2026 09:23
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

plugins/pipelinesteps/plugin.go:145

  • app.RegisterService returns an error (e.g., duplicate service name). Ignoring it here can silently break post-start wiring that relies on this service being present. Consider handling the error and returning it from the wiring hook (or at least logging it) so BuildFromConfig fails fast if the handler service cannot be registered.
				// Register the handler as a service so callers can discover it
				// (e.g. to wire SetEventRecorder post-start) without a plugin-specific getter.
				_ = app.RegisterService(PipelineHandlerServiceName, p.pipelineHandler)
				return nil

The handlers package import was left behind after PR changes removed all
usage of the package from cmd/server/main.go, causing build and lint failures.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@intel352 intel352 merged commit 4caa62e into main Feb 23, 2026
14 checks passed
@intel352 intel352 deleted the refactor/issue-60-remove-plugin-handler-getter branch February 23, 2026 09:50
intel352 added a commit that referenced this pull request May 15, 2026
…r module factories) (#682)

* docs: design — extend ServeIaCPlugin to also serve module + step factories

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>

* docs: apply cycle-1 adversarial-review fixes

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>

* docs: cycle-2 adversarial-review fixes (PASS-with-acceptance)

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>

* docs: plugin-modules-on-iac implementation plan (5 PRs / 19 tasks)

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>

* docs: apply cycle-1 adversarial-review fixes to plan

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>

* docs: cycle-2 adversarial-review fix (PASS-with-acceptance)

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>

* chore: lock scope for plugin-modules-on-iac (alignment passed)

* docs: address Copilot precision findings on #682 (status line + ADR shim wording)

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

Plugin exposes concrete handler type via getter (plugins/pipelinesteps/plugin.go)

2 participants