refactor: use interfaces.PipelineRunner in RoutePipelineSetter by intel352 · Pull Request #116 · GoCodeAlone/workflow

intel352 · 2026-02-23T08:15:35Z

Summary

Updates RoutePipelineSetter interface in engine.go to use interfaces.PipelineRunner instead of *module.Pipeline
Updates CommandHandler and QueryHandler in module/ to store and accept interfaces.PipelineRunner in their routePipelines maps and SetRoutePipeline methods
ServeHTTP in both handlers type-asserts to *Pipeline for concrete field access (Metadata, RoutePattern, Execute) with a Run()-based fallback for non-*Pipeline implementations

*module.Pipeline already satisfies PipelineRunner, so engine call sites pass *module.Pipeline unchanged — no call-site changes required there.

Closes #58

Test plan

go build ./... passes
go test ./... passes (all packages)
golangci-lint run reports 0 issues (verified by pre-push hook)
go fmt ./... applied

🤖 Generated with Claude Code

…#58) Replace the concrete *module.Pipeline type in the RoutePipelineSetter interface and the CommandHandler/QueryHandler routePipelines map with interfaces.PipelineRunner. This decouples the engine's route-pipeline wiring contract from the concrete Pipeline implementation, allowing test doubles and future plugin-provided pipelines to satisfy the interface without importing the module package. Changes: - engine.go: RoutePipelineSetter.SetRoutePipeline now takes interfaces.PipelineRunner - module/command_handler.go: routePipelines map and SetRoutePipeline use interfaces.PipelineRunner; ServeHTTP type-asserts to *Pipeline for concrete field access (Metadata, RoutePattern, Execute) with a Run()-based fallback for non-*Pipeline implementations - module/query_handler.go: same as command_handler *module.Pipeline already satisfies PipelineRunner, so engine.go callers pass *module.Pipeline unchanged — no call-site changes required there. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Refactors per-route pipeline wiring to depend on the interfaces.PipelineRunner abstraction instead of the concrete *module.Pipeline, improving extensibility and reducing package coupling across the workflow engine and HTTP handlers.

Changes:

Updated RoutePipelineSetter to accept interfaces.PipelineRunner instead of *module.Pipeline.
Updated CommandHandler/QueryHandler to store per-route pipelines as interfaces.PipelineRunner, with a *Pipeline fast-path and Run() fallback.
Applied minor gofmt-only formatting updates in tests.

Reviewed changes

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

Show a summary per file

File	Description
`engine.go`	Switches `RoutePipelineSetter` to accept `interfaces.PipelineRunner`.
`module/query_handler.go`	Stores route pipelines as `PipelineRunner`; adds `*Pipeline` type-assert path + `Run()` fallback.
`module/command_handler.go`	Stores route pipelines as `PipelineRunner`; adds `*Pipeline` type-assert path + `Run()` fallback.
`plugins/http/plugin_test.go`	gofmt alignment change only.
`cmd/server/main_test.go`	gofmt formatting change only.

Copilot · 2026-02-23T08:19:03Z

module/command_handler.go

+		// Type-assert to *Pipeline for concrete field access (Metadata, RoutePattern,
+		// Execute) and execution tracker integration. All engine-registered pipelines
+		// are *Pipeline; the interface allows custom implementations in tests/plugins.
+		if concretePipeline, ok := pipeline.(*Pipeline); ok {
+			// Inject HTTP context so delegate steps can forward directly
+			concretePipeline.Metadata = map[string]any{
+				"_http_request":         r,
+				"_http_response_writer": w,
+			}


routePipelines is now a map[string]interfaces.PipelineRunner, so it can contain a non-nil interface value whose dynamic value is a typed-nil pointer. In that case pipeline != nil will be true, the type assertion will succeed, and concretePipeline.Metadata = ... will panic due to nil dereference. Add a guard for typed-nil (e.g., check concretePipeline == nil after the assertion, or normalize typed-nil to nil in SetRoutePipeline).

Copilot · 2026-02-23T08:19:03Z

module/command_handler.go

+		// Fallback for non-*Pipeline implementations: use the PipelineRunner interface.
+		result, err := pipeline.Run(r.Context(), triggerData)
+		if err != nil {
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusInternalServerError)
+			_ = json.NewEncoder(w).Encode(map[string]string{"error": err.Error()})
 			return
 		}
 		w.Header().Set("Content-Type", "application/json")
-		if err := json.NewEncoder(w).Encode(pc.Current); err != nil {
+		if err := json.NewEncoder(w).Encode(result); err != nil {
 			http.Error(w, "failed to encode response", http.StatusInternalServerError)
 		}
 		return


For the non-*Pipeline fallback (PipelineRunner.Run), the handler always JSON-encodes the returned map and has no equivalent of the _response_handled metadata guard used in the *Pipeline path. This means non-*Pipeline implementations cannot safely write to the ResponseWriter (or use built-in steps like delegate/json_response that rely on pc.Metadata) without risking double responses. Consider extending the interface/contract so the runner can signal "response already written" (or have Run return a richer result that includes this flag).

Copilot · 2026-02-23T08:19:03Z

module/command_handler.go

+		// Type-assert to *Pipeline for concrete field access (Metadata, RoutePattern,
+		// Execute) and execution tracker integration. All engine-registered pipelines
+		// are *Pipeline; the interface allows custom implementations in tests/plugins.
+		if concretePipeline, ok := pipeline.(*Pipeline); ok {
+			// Inject HTTP context so delegate steps can forward directly
+			concretePipeline.Metadata = map[string]any{
+				"_http_request":         r,
+				"_http_response_writer": w,
+			}
+			if concretePipeline.RoutePattern != "" {
+				concretePipeline.Metadata["_route_pattern"] = concretePipeline.RoutePattern
+			}
+			var pc *PipelineContext
+			var err error
+			if h.executionTracker != nil {
+				pc, err = h.executionTracker.TrackPipelineExecution(r.Context(), concretePipeline, triggerData, r)
+			} else {
+				pc, err = concretePipeline.Execute(r.Context(), triggerData)
+			}
+			if err != nil {
+				if pc == nil || pc.Metadata["_response_handled"] != true {
+					w.Header().Set("Content-Type", "application/json")
+					w.WriteHeader(http.StatusInternalServerError)
+					_ = json.NewEncoder(w).Encode(map[string]string{"error": err.Error()})
+				}
+				return
+			}
+			if pc.Metadata["_response_handled"] == true {
+				return
+			}
+			w.Header().Set("Content-Type", "application/json")
+			if err := json.NewEncoder(w).Encode(pc.Current); err != nil {
+				http.Error(w, "failed to encode response", http.StatusInternalServerError)
 			}
 			return
 		}
-		if pc.Metadata["_response_handled"] == true {
+		// Fallback for non-*Pipeline implementations: use the PipelineRunner interface.
+		result, err := pipeline.Run(r.Context(), triggerData)
+		if err != nil {


The new non-*Pipeline fallback path (PipelineRunner.Run) and the typed-assertion branch behavior aren’t covered by the existing CommandHandler unit tests (there are tests for command dispatch/delegation, but none that attach a route pipeline). Add tests that: (1) attach a mock PipelineRunner and verify the Run result is returned, and (2) attach a typed-nil runner to ensure it’s handled without panicking.

Copilot · 2026-02-23T08:19:03Z

module/query_handler.go

+			// Inject HTTP context so delegate steps can forward directly
+			concretePipeline.Metadata = map[string]any{
+				"_http_request":         r,
+				"_http_response_writer": w,
+			}
+			if concretePipeline.RoutePattern != "" {
+				concretePipeline.Metadata["_route_pattern"] = concretePipeline.RoutePattern
+			}
+			var pc *PipelineContext
+			var err error
+			if h.executionTracker != nil {
+				pc, err = h.executionTracker.TrackPipelineExecution(r.Context(), concretePipeline, triggerData, r)
+			} else {
+				pc, err = concretePipeline.Execute(r.Context(), triggerData)
+			}
+			if err != nil {
+				// Only write error if response wasn't already handled by a delegate step
+				if pc == nil || pc.Metadata["_response_handled"] != true {
+					w.Header().Set("Content-Type", "application/json")
+					w.WriteHeader(http.StatusInternalServerError)
+					_ = json.NewEncoder(w).Encode(map[string]string{"error": err.Error()})
+				}
+				return
+			}
+			// If response was handled by a delegate step, don't write again
+			if pc.Metadata["_response_handled"] == true {
+				return
+			}
+			w.Header().Set("Content-Type", "application/json")
+			if err := json.NewEncoder(w).Encode(pc.Current); err != nil {
+				http.Error(w, "failed to encode response", http.StatusInternalServerError)
 			}
 			return


routePipelines is now a map[string]interfaces.PipelineRunner, so it can contain a non-nil interface value whose dynamic value is a typed-nil pointer. In that case pipeline != nil will be true, the type assertion will succeed, and concretePipeline.Metadata = ... will panic due to nil dereference. Add a guard for typed-nil (e.g., check concretePipeline == nil after the assertion, or normalize typed-nil to nil in SetRoutePipeline).

Suggested change

// Inject HTTP context so delegate steps can forward directly

concretePipeline.Metadata = map[string]any{

"_http_request": r,

"_http_response_writer": w,

}

if concretePipeline.RoutePattern != "" {

concretePipeline.Metadata["_route_pattern"] = concretePipeline.RoutePattern

}

var pc *PipelineContext

var err error

if h.executionTracker != nil {

pc, err = h.executionTracker.TrackPipelineExecution(r.Context(), concretePipeline, triggerData, r)

} else {

pc, err = concretePipeline.Execute(r.Context(), triggerData)

}

if err != nil {

// Only write error if response wasn't already handled by a delegate step

if pc == nil || pc.Metadata["_response_handled"] != true {

w.Header().Set("Content-Type", "application/json")

w.WriteHeader(http.StatusInternalServerError)

_ = json.NewEncoder(w).Encode(map[string]string{"error": err.Error()})

}

return

}

// If response was handled by a delegate step, don't write again

if pc.Metadata["_response_handled"] == true {

return

}

w.Header().Set("Content-Type", "application/json")

if err := json.NewEncoder(w).Encode(pc.Current); err != nil {

http.Error(w, "failed to encode response", http.StatusInternalServerError)

}

return

// Guard against typed-nil *Pipeline stored in the interface.

if concretePipeline == nil {

// Treat typed-nil as no pipeline and fall through to delegate/404 handling.

} else {

// Inject HTTP context so delegate steps can forward directly

concretePipeline.Metadata = map[string]any{

"_http_request": r,

"_http_response_writer": w,

}

if concretePipeline.RoutePattern != "" {

concretePipeline.Metadata["_route_pattern"] = concretePipeline.RoutePattern

}

var pc *PipelineContext

var err error

if h.executionTracker != nil {

pc, err = h.executionTracker.TrackPipelineExecution(r.Context(), concretePipeline, triggerData, r)

} else {

pc, err = concretePipeline.Execute(r.Context(), triggerData)

}

if err != nil {

// Only write error if response wasn't already handled by a delegate step

if pc == nil || pc.Metadata["_response_handled"] != true {

w.Header().Set("Content-Type", "application/json")

w.WriteHeader(http.StatusInternalServerError)

_ = json.NewEncoder(w).Encode(map[string]string{"error": err.Error()})

}

return

}

// If response was handled by a delegate step, don't write again

if pc.Metadata["_response_handled"] == true {

return

}

w.Header().Set("Content-Type", "application/json")

if err := json.NewEncoder(w).Encode(pc.Current); err != nil {

http.Error(w, "failed to encode response", http.StatusInternalServerError)

}

return

}

Copilot · 2026-02-23T08:19:04Z

module/query_handler.go

+		// Fallback for non-*Pipeline implementations: use the PipelineRunner interface.
+		result, err := pipeline.Run(r.Context(), triggerData)
+		if err != nil {
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusInternalServerError)
+			_ = json.NewEncoder(w).Encode(map[string]string{"error": err.Error()})
 			return
 		}
 		w.Header().Set("Content-Type", "application/json")
-		if err := json.NewEncoder(w).Encode(pc.Current); err != nil {
+		if err := json.NewEncoder(w).Encode(result); err != nil {
 			http.Error(w, "failed to encode response", http.StatusInternalServerError)
 		}
 		return


For the non-*Pipeline fallback (PipelineRunner.Run), the handler always JSON-encodes the returned map and has no equivalent of the _response_handled metadata guard used in the *Pipeline path. This means non-*Pipeline implementations cannot safely write to the ResponseWriter (or use built-in steps like delegate/json_response that rely on pc.Metadata) without risking double responses. Consider extending the interface/contract so the runner can signal "response already written" (or have Run return a richer result that includes this flag).

Copilot · 2026-02-23T08:19:04Z

module/query_handler.go

+		// Type-assert to *Pipeline for concrete field access (Metadata, RoutePattern,
+		// Execute) and execution tracker integration. All engine-registered pipelines
+		// are *Pipeline; the interface allows custom implementations in tests/plugins.
+		if concretePipeline, ok := pipeline.(*Pipeline); ok {
+			// Inject HTTP context so delegate steps can forward directly
+			concretePipeline.Metadata = map[string]any{
+				"_http_request":         r,
+				"_http_response_writer": w,
+			}
+			if concretePipeline.RoutePattern != "" {
+				concretePipeline.Metadata["_route_pattern"] = concretePipeline.RoutePattern
+			}
+			var pc *PipelineContext
+			var err error
+			if h.executionTracker != nil {
+				pc, err = h.executionTracker.TrackPipelineExecution(r.Context(), concretePipeline, triggerData, r)
+			} else {
+				pc, err = concretePipeline.Execute(r.Context(), triggerData)
+			}
+			if err != nil {
+				// Only write error if response wasn't already handled by a delegate step
+				if pc == nil || pc.Metadata["_response_handled"] != true {
+					w.Header().Set("Content-Type", "application/json")
+					w.WriteHeader(http.StatusInternalServerError)
+					_ = json.NewEncoder(w).Encode(map[string]string{"error": err.Error()})
+				}
+				return
+			}
+			// If response was handled by a delegate step, don't write again
+			if pc.Metadata["_response_handled"] == true {
+				return
+			}
+			w.Header().Set("Content-Type", "application/json")
+			if err := json.NewEncoder(w).Encode(pc.Current); err != nil {
+				http.Error(w, "failed to encode response", http.StatusInternalServerError)
 			}
 			return
 		}
-		// If response was handled by a delegate step, don't write again
-		if pc.Metadata["_response_handled"] == true {
+		// Fallback for non-*Pipeline implementations: use the PipelineRunner interface.
+		result, err := pipeline.Run(r.Context(), triggerData)
+		if err != nil {


The new non-*Pipeline fallback path (PipelineRunner.Run) and the typed-assertion branch behavior aren’t covered by the existing QueryHandler unit tests (there are tests for query dispatch/delegation, but none that attach a route pipeline). Add tests that: (1) attach a mock PipelineRunner and verify the Run result is returned, and (2) attach a typed-nil runner to ensure it’s handled without panicking.

…rface

intel352 · 2026-02-23T08:57:51Z

@copilot open a new pull request to apply changes based on the comments in this thread

Copilot · 2026-02-23T08:57:59Z

@intel352 I've opened a new pull request, #121, to work on those changes. Once the pull request is ready, I'll request review from you.

…fallback path (#121) * Initial plan * fix: typed-nil guard, _response_handled fallback, add route pipeline tests Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

Copilot

Pull request overview

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

Copilot AI review requested due to automatic review settings February 23, 2026 08:15

Copilot started reviewing on behalf of intel352 February 23, 2026 08:16 View session

Copilot AI reviewed Feb 23, 2026

View reviewed changes

Merge branch 'main' into refactor/issue-58-route-pipeline-setter-inte…

8b052de

…rface

Copilot AI mentioned this pull request Feb 23, 2026

fix: typed-nil guard and _response_handled parity for PipelineRunner fallback path #121

Merged

Copilot AI review requested due to automatic review settings February 23, 2026 09:19

Copilot started reviewing on behalf of intel352 February 23, 2026 09:19 View session

Copilot AI reviewed Feb 23, 2026

View reviewed changes

intel352 merged commit 9fc5d0b into main Feb 23, 2026
18 checks passed

intel352 deleted the refactor/issue-58-route-pipeline-setter-interface branch February 23, 2026 09:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use interfaces.PipelineRunner in RoutePipelineSetter#116

refactor: use interfaces.PipelineRunner in RoutePipelineSetter#116
intel352 merged 3 commits intomainfrom
refactor/issue-58-route-pipeline-setter-interface

intel352 commented Feb 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 23, 2026

Uh oh!

Copilot AI Feb 23, 2026

Uh oh!

Copilot AI Feb 23, 2026

Uh oh!

Copilot AI Feb 23, 2026

Uh oh!

Copilot AI Feb 23, 2026

Uh oh!

Copilot AI Feb 23, 2026

Uh oh!

intel352 commented Feb 23, 2026

Uh oh!

Copilot AI commented Feb 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

intel352 commented Feb 23, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

intel352 commented Feb 23, 2026

Uh oh!

Copilot AI commented Feb 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants