Conversation
- Add handlers/cli.go: CLIWorkflowHandler for 'cli' workflow type with command registration, config-driven dispatch, and usage generation - Add handlers/cli_test.go: comprehensive tests for CLIWorkflowHandler - Add config.LoadFromBytes helper for loading configs from embedded bytes - Register CLIWorkflowHandler in setup.DefaultHandlers() - Add cmd/wfctl/wfctl.yaml: embedded CLI config declaring all 24 commands - Rewrite cmd/wfctl/main.go to load embedded config and use CLIWorkflowHandler for dispatch, removing hardcoded usage() and command routing Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Evaluate workflow engine for CLI app support and extend functionality
Add CLI workflow handler and convert wfctl to a workflow config embedded app
Mar 7, 2026
Contributor
|
@copilot convert the remaining cli commands to workflow primitives. Extend the engine where necessary for new functionality that makes sense existing in the engine. |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
…oke, step.cli_print Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Contributor
Author
Done in commit 4e00330. Here's what was added: New engine primitives:
wfctl is now fully pipeline-driven: Pre/post steps (audit, telemetry, feature flags) can now be added to any command pipeline in YAML without touching Go code. Verified: all 24 commands work, all tests pass, zero lint issues. |
Copilot
AI
changed the title
Add CLI workflow handler and convert wfctl to a workflow config embedded app
Add CLI workflow handler and convert wfctl to a workflow config embedded app with full pipeline primitives
Mar 7, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Adds first-class CLI workflows to the workflow engine so CLI commands can be configured and executed as pipelines, and rewires wfctl to dispatch through the engine using an embedded YAML config.
Changes:
- Introduces
handlers/cli.go(CLIWorkflowHandler) andmodule/cli_trigger.go(trigger.cli) to supportworkflows.cliand CLI-triggered pipelines. - Adds CLI pipeline primitives:
step.cli_printandstep.cli_invoke, plus a sharedCLICommandRegistryservice for Go-backed command execution. - Converts
wfctlto an embedded-config, workflow-driven CLI where each command is a pipeline triggered viatype: cli.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| setup/setup.go | Registers the new CLI workflow handler and CLI trigger in defaults. |
| setup/setup_test.go | Updates expected default handler/trigger counts. |
| plugins/pipelinesteps/plugin.go | Registers step.cli_print and step.cli_invoke step types/factories. |
| plugins/pipelinesteps/plugin_test.go | Ensures new CLI step types are included in the plugin step factory list. |
| module/pipeline_step_cli_print.go | Implements step.cli_print (templated stdout/stderr output). |
| module/pipeline_step_cli_print_test.go | Adds unit tests for step.cli_print. |
| module/pipeline_step_cli_invoke.go | Implements step.cli_invoke (invokes a Go command from CLICommandRegistry). |
| module/pipeline_step_cli_invoke_test.go | Adds unit tests for step.cli_invoke. |
| module/cli_trigger.go | Implements trigger.cli mapping CLI commands to pipeline:<name> workflows. |
| module/cli_trigger_test.go | Adds unit tests for CLITrigger. |
| module/cli_command_registry.go | Adds the shared CLI command registry service used by step.cli_invoke. |
| handlers/cli.go | Implements CLIWorkflowHandler for workflows.cli with usage/version/help + dispatch. |
| handlers/cli_test.go | Adds unit tests covering dispatch, help/version, handler keys, and pipeline fallback. |
| config/config.go | Adds config.LoadFromBytes([]byte) for embedded YAML configs. |
| cmd/wfctl/wfctl.yaml | Declares workflows.cli commands and maps each to a CLI-triggered command pipeline. |
| cmd/wfctl/type_registry.go | Exposes new CLI step types in wfctl’s type registry metadata. |
| cmd/wfctl/main.go | Embeds wfctl.yaml, builds/configures engine, registers registry + CLI steps, dispatches via CLIWorkflowHandler. |
handlers/cli.go
Outdated
Comment on lines
+119
to
+124
| func (h *CLIWorkflowHandler) ExecuteWorkflow(_ context.Context, _ string, action string, data map[string]any) (map[string]any, error) { | ||
| args, _ := data["args"].([]string) | ||
| if err := h.runCommand(action, args); err != nil { | ||
| return nil, err | ||
| } | ||
| return map[string]any{"success": true}, nil |
There was a problem hiding this comment.
CLIWorkflowHandler.ExecuteWorkflow ignores the provided context, and pipeline-backed commands end up being dispatched with context.Background() (see runCommand). This drops cancellation/timeouts/tracing for programmatic invocations; thread the ctx through to runCommand and into CLIPipelineDispatcher.DispatchCommand instead of discarding it.
Comment on lines
+256
to
+267
| if rawCmds, ok := cfgMap["commands"].([]any); ok { | ||
| for i, rc := range rawCmds { | ||
| cmdMap, ok := rc.(map[string]any) | ||
| if !ok { | ||
| return nil, fmt.Errorf("command at index %d is not a map", i) | ||
| } | ||
| def := CLICommandDef{} | ||
| def.Name, _ = cmdMap["name"].(string) | ||
| def.Description, _ = cmdMap["description"].(string) | ||
| def.Handler, _ = cmdMap["handler"].(string) | ||
| cfg.Commands = append(cfg.Commands, def) | ||
| } |
There was a problem hiding this comment.
parseCLIWorkflowConfig accepts commands with an empty or missing name and appends them, which later produces a map entry under the empty string key. This makes dispatch/usage output confusing and can hide misconfigurations. Validate that each command has a non-empty name (and ideally reject duplicates) while parsing or during ConfigureWorkflow.
Comment on lines
+178
to
+183
| // Fallback: pipeline dispatch via CLITrigger found in app services. | ||
| if h.app != nil { | ||
| for _, svc := range h.app.SvcRegistry() { | ||
| if d, ok := svc.(CLIPipelineDispatcher); ok { | ||
| return d.DispatchCommand(context.Background(), name, args) | ||
| } |
There was a problem hiding this comment.
Pipeline dispatch uses context.Background(), which prevents CLI-triggered pipeline executions from being cancellable (e.g., Ctrl+C) and drops any upstream context values. After threading ctx through ExecuteWorkflow/Dispatch, pass that ctx into DispatchCommand instead of creating a new background context here.
| if workflowType == "" { | ||
| return fmt.Errorf("cli trigger: 'workflowType' is required in trigger config (injected by the engine)") | ||
| } | ||
|
|
There was a problem hiding this comment.
CLITrigger.Configure overwrites an existing command→workflow mapping without warning. If multiple pipelines accidentally declare the same command, the last one wins silently. Return an error (or at least log) when a different workflowType is already registered for the command to avoid ambiguous CLI routing.
Suggested change
| // Prevent ambiguous CLI routing: do not silently overwrite an existing | |
| // command → workflowType mapping with a different workflow. Re-registering | |
| // the same mapping is allowed for idempotency. | |
| if existing, ok := t.commands[command]; ok && existing != workflowType { | |
| return fmt.Errorf("cli trigger: command %q already registered for workflow %q (cannot re-register for %q)", command, existing, workflowType) | |
| } |
handlers/cli_test.go
Outdated
Comment on lines
+315
to
+326
| // containsStr is a simple substring helper. | ||
| func containsStr(s, sub string) bool { | ||
| return len(sub) == 0 || (len(s) >= len(sub) && (s == sub || len(s) > 0 && | ||
| func() bool { | ||
| for i := 0; i <= len(s)-len(sub); i++ { | ||
| if s[i:i+len(sub)] == sub { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| }())) | ||
| } |
There was a problem hiding this comment.
The containsStr helper reimplements substring search manually, which is harder to read and maintain in tests. Prefer using strings.Contains (or bytes.Contains) for clarity and to avoid subtle edge-case bugs.
handlers/cli.go
Outdated
| h.app = app | ||
| // Register self so engine consumers can retrieve this handler by name. | ||
| if app != nil { | ||
| _ = app.RegisterService(CLIWorkflowHandlerServiceName, h) |
There was a problem hiding this comment.
RegisterService errors are ignored unconditionally here. If some other service already occupies "cliWorkflowHandler", this will silently fail and callers may retrieve the wrong handler (or none). Consider checking the error and only ignoring the specific "already registered" case when the existing service is the same instance, otherwise return an error.
Suggested change
| _ = app.RegisterService(CLIWorkflowHandlerServiceName, h) | |
| if err := app.RegisterService(CLIWorkflowHandlerServiceName, h); err != nil { | |
| return fmt.Errorf("cli workflow: register service %q: %w", CLIWorkflowHandlerServiceName, err) | |
| } |
module/cli_trigger.go
Outdated
Comment on lines
+77
to
+79
| // RegisterService may return an error if the name is already taken, but | ||
| // since we re-register the *same* pointer this is idempotent and safe to ignore. | ||
| _ = app.RegisterService(t.name, t) |
There was a problem hiding this comment.
CLITrigger.Configure ignores the error from RegisterService. If "trigger.cli" is already registered to a different instance, this will silently keep the old service and dispatch may route to unexpected pipelines. Consider checking for an existing service and only ignoring the error when the existing value is the same *CLITrigger; otherwise return an error.
Suggested change
| // RegisterService may return an error if the name is already taken, but | |
| // since we re-register the *same* pointer this is idempotent and safe to ignore. | |
| _ = app.RegisterService(t.name, t) | |
| // If a service is already registered under this name, only tolerate it when | |
| // it is the same *CLITrigger instance; otherwise surface an error so | |
| // configuration fails instead of silently using the wrong trigger. | |
| if err := app.RegisterService(t.name, t); err != nil { | |
| // Check whether this instance is already present in the service registry. | |
| var sameInstanceAlreadyRegistered bool | |
| for _, svc := range app.SvcRegistry() { | |
| if existing, ok := svc.(*CLITrigger); ok && existing == t { | |
| sameInstanceAlreadyRegistered = true | |
| break | |
| } | |
| } | |
| if !sameInstanceAlreadyRegistered { | |
| return fmt.Errorf("cli trigger: registering service %q: %w", t.name, err) | |
| } | |
| } |
module/pipeline_step_cli_print.go
Outdated
Comment on lines
+49
to
+50
| if t, ok := config["target"].(string); ok && t == "stderr" { | ||
| target = stderr |
There was a problem hiding this comment.
If config["target"] is set to an unrecognized value, this silently defaults to stdout. That makes misconfigurations hard to spot and can leak error output to stdout unexpectedly. Validate target is either "stdout" or "stderr" and return a config error for any other non-empty value.
Suggested change
| if t, ok := config["target"].(string); ok && t == "stderr" { | |
| target = stderr | |
| if rawTarget, ok := config["target"]; ok { | |
| if t, ok := rawTarget.(string); ok && t != "" { | |
| switch t { | |
| case "stdout": | |
| target = stdout | |
| case "stderr": | |
| target = stderr | |
| default: | |
| return nil, fmt.Errorf("cli_print step %q: invalid 'target' %q; must be \"stdout\" or \"stderr\"", name, t) | |
| } | |
| } |
Comment on lines
+63
to
+65
| // Extract args from the pipeline context (injected by CLITrigger.DispatchCommand). | ||
| args, _ := pc.Current["args"].([]string) | ||
|
|
There was a problem hiding this comment.
CLIInvokeStep only accepts pc.Current["args"] as a []string; if the trigger data came from JSON/YAML decoding it will often be []any, and this will silently pass nil args to the command. Consider supporting []any (convert to []string) and/or returning a clear error when args is present but not a string slice.
cmd/wfctl/main.go
Outdated
Comment on lines
+77
to
+80
| // Suppress pipeline execution logs globally: wfctl is a CLI tool and | ||
| // internal pipeline step/run logs should not leak to the user's terminal. | ||
| // Each command creates its own logger when it needs output. | ||
| slog.SetDefault(engineLogger) |
There was a problem hiding this comment.
slog.SetDefault(engineLogger) changes the process-wide default logger to discard output. This can unintentionally suppress logs from subcommands (e.g., cmd/wfctl/plugin_test_cmd.go uses slog.Default() when not in --verbose mode) and from any dependency relying on slog.Default(). Prefer keeping the discard logger scoped to the engine (WithLogger) and avoid mutating the global default, or restore the previous default after engine startup/config.
Suggested change
| // Suppress pipeline execution logs globally: wfctl is a CLI tool and | |
| // internal pipeline step/run logs should not leak to the user's terminal. | |
| // Each command creates its own logger when it needs output. | |
| slog.SetDefault(engineLogger) | |
| // Suppress pipeline execution logs within the engine: wfctl is a CLI tool | |
| // and internal pipeline step/run logs should not leak to the user's terminal. | |
| // Each command creates its own logger when it needs output. |
Contributor
|
@copilot apply changes based on the comments in this thread |
…, engine logger propagation Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Contributor
Author
Applied all review feedback in commit fe50444:
|
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.
The workflow engine had no support for CLI app workflows. This adds a
cliworkflow handler type, aclitrigger type, and CLI-specific pipeline steps — then rewireswfctlso every command is a first-class workflow pipeline triggered by the engine rather than a direct Go function call.New:
handlers/cli.go—CLIWorkflowHandler"cli"workflow typeCLIWorkflowConfigdefines app metadata + a command list (name, description, optional handler key)RegisterCommand(key, fn CLICommandFunc)wires Go implementations to config-declared commandsDispatch(args []string)/DispatchContext(ctx, args)handle--help/--versionflags, unknown-command errors, usage generation, and command routing;DispatchContextaccepts an explicit context for cancellation and tracing propagationExecuteWorkflowthreads the provided context through to the pipeline dispatcher so cancellation and tracing signals are preserved for programmatic invocationsConfigureWorkflowfully reinitialises the command index on each call so that reconfigured or removed commands never lingerRegisterServiceerrors are surfaced unless the same handler instance is already registered (idempotent re-registration)"cliWorkflowHandler"service duringConfigureWorkflowso callers can retrieve it from the app service registryCLIPipelineDispatcher(implemented byCLITrigger) when no direct Go runner is registered — enabling fully pipeline-backed commandsNew:
module/cli_trigger.go—CLITrigger(trigger.cli)DefaultTriggers()(now 6 built-in triggers)type: cli; accumulatescommand → pipeline:namemappings across multiple callsworkflowTypefor an already-mapped command returns an error; re-registering the exact same mapping is idempotentRegisterServiceerrors are surfaced unless the same*CLITriggerinstance is already registeredDispatchCommand(ctx, cmd, args)callsengine.TriggerWorkflowsynchronously, threading the caller's context through"trigger.cli"service duringConfiguresoCLIWorkflowHandlercan discover it lazily from the appNew:
module/cli_command_registry.go—CLICommandRegistry"cliCommandRegistry") that maps CLI command names toCLICommandFuncGo implementationsBuildFromConfigsostep.cli_invokecan resolve functions at pipeline execution timeNew:
step.cli_printmessage(required),newline(default true),target("stdout"|"stderr") — unknown target values return a clear config errorNew:
step.cli_invokeCLICommandRegistrypc.Current["args"]as either[]string(direct Go call) or[]any(JSON/YAML round-trip), with a clear error for non-string elementsplugins/pipelinestepsEngine: pipeline logger propagation (
engine.go)configurePipelinesnow injects the engine's*slog.Loggerdirectly into eachPipelineit creates. This meansWithLogger(discard)fully suppresses pipeline execution logs without requiring callers to mutate the globalslog.Default().config.LoadFromBytesNew helper to parse a
WorkflowConfigfrom an in-memory[]byte— the primitive needed for//go:embedconfigs.setup.DefaultHandlers/setup.DefaultTriggersCLIWorkflowHandleradded toDefaultHandlersandCLITriggeradded toDefaultTriggers, both available viaWithAllDefaults().cmd/wfctl— every command is now a workflow pipelinewfctl.yamlnow declares apipelinessection with 24cmd-<name>pipelines, each wired to aclitrigger and astep.cli_invokestep.main.gobuilds a full engine, registers aCLICommandRegistrywith all Go implementations, callsBuildFromConfig, and retrieves theCLIWorkflowHandlerfrom the app service registry for dispatch.main.gousessignal.NotifyContext+DispatchContextso Ctrl+C cancels the running pipeline cleanly. The engine's discard logger is scoped entirely within the engine (noslog.SetDefaultmutation).The complete dispatch chain for every command is:
Pre/post steps (audit logging, telemetry, feature flags) can now be added to any command pipeline in YAML without modifying Go code.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.