From f6d6c4d867d07b93351484f5c9bbd9ef9b8408ae Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Mon, 15 Jun 2026 12:33:45 -0500 Subject: [PATCH 1/3] feat(cli): add plugin invocation protocol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C1 (osi convert base) needs a subprocess layer to pipe requests to plugins and receive responses. This adds the Request/Response/Issue envelope types and an Invoke() function that owns the full lifecycle: marshal request to stdin, capture stdout, forward stderr, enforce context timeout, and decode the JSON response. Design decisions: - Invoke takes pluginDir and invoke []string directly rather than *Plugin — decouples the invocation layer from the discovery types and makes the function independently testable - cmd.Stdout assigned as *bytes.Buffer rather than using cmd.Output() — cmd.Output() is incompatible with a pre-assigned cmd.Stderr writer and would silently discard the caller's stderr destination - ctx.Err() checked after cmd.Run() for timeout detection — exec kills the process with SIGKILL and returns "signal: killed", not context.DeadlineExceeded, so the context must be checked explicitly - Issue.Path uses omitempty — an absent key is semantically distinct from an empty string; plugins that have no path omit the field - A non-empty Issues slice is NOT a Go error — the caller (C1) is responsible for inspecting severities and determining exit code Tests use the TestMain subprocess self-invocation pattern: the test binary re-invokes itself with GO_TEST_PLUGIN=1, enters the fake plugin branch in TestMain, and exits without running any tests. This avoids compiling a separate helper binary and works portably. Caveats: - File collection, output writing, issue rendering, and wiring into cmd/convert.go are deferred to C1 --- cli/internal/plugin/invoke.go | 74 ++++++++ cli/internal/plugin/invoke_test.go | 263 +++++++++++++++++++++++++++++ 2 files changed, 337 insertions(+) create mode 100644 cli/internal/plugin/invoke.go create mode 100644 cli/internal/plugin/invoke_test.go diff --git a/cli/internal/plugin/invoke.go b/cli/internal/plugin/invoke.go new file mode 100644 index 00000000..d7e4018a --- /dev/null +++ b/cli/internal/plugin/invoke.go @@ -0,0 +1,74 @@ +package plugin + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "os/exec" +) + +// Request is the JSON envelope sent to a plugin on stdin. +type Request struct { + Files map[string]string `json:"files"` +} + +// Response is the JSON envelope received from a plugin on stdout. +type Response struct { + Files map[string]string `json:"files"` + Issues []Issue `json:"issues,omitempty"` +} + +// Issue is a single diagnostic emitted by a plugin during conversion. +// Severity is one of "error", "warning", or "info". +// Path is optional; it is omitted for issues not tied to a specific location. +type Issue struct { + Severity string `json:"severity"` + Message string `json:"message"` + Path string `json:"path,omitempty"` +} + +// Invoke runs a plugin subprocess, pipes req as JSON to its stdin, +// and decodes its stdout as a JSON Response. +// +// pluginDir is used as the working directory for the subprocess so that +// relative paths in the invoke array resolve correctly. +// invoke is the command and its arguments (invoke[0] is the executable). +// pluginStderr receives the plugin's stderr verbatim; pass io.Discard to suppress. +// +// Hard errors: failed to marshal request, process start failure, context +// deadline exceeded, non-zero exit code, or invalid JSON in the response. +// A non-empty Issues slice in the response is NOT itself a Go error — the +// caller is responsible for inspecting severities and setting the exit code. +func Invoke(ctx context.Context, pluginDir string, invoke []string, req Request, pluginStderr io.Writer) (*Response, error) { + reqJSON, err := json.Marshal(req) + if err != nil { + return nil, fmt.Errorf("failed to marshal plugin request: %w", err) + } + + var stdout bytes.Buffer + cmd := exec.CommandContext(ctx, invoke[0], invoke[1:]...) + cmd.Dir = pluginDir + cmd.Stdin = bytes.NewReader(reqJSON) + // Use an explicit buffer rather than cmd.Output() — cmd.Output() is + // incompatible with a pre-assigned cmd.Stderr writer. + cmd.Stdout = &stdout + cmd.Stderr = pluginStderr + + if err := cmd.Run(); err != nil { + // ctx.Err() is the authoritative signal for timeout/cancellation. + // exec.CommandContext kills the process and returns "signal: killed", + // not context.DeadlineExceeded, so we must check ctx.Err() explicitly. + if ctx.Err() != nil { + return nil, fmt.Errorf("plugin timed out: %w", ctx.Err()) + } + return nil, fmt.Errorf("plugin process failed: %w", err) + } + + var resp Response + if err := json.Unmarshal(stdout.Bytes(), &resp); err != nil { + return nil, fmt.Errorf("invalid JSON response from plugin: %w", err) + } + return &resp, nil +} diff --git a/cli/internal/plugin/invoke_test.go b/cli/internal/plugin/invoke_test.go new file mode 100644 index 00000000..9345094b --- /dev/null +++ b/cli/internal/plugin/invoke_test.go @@ -0,0 +1,263 @@ +package plugin_test + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "os" + "strings" + "testing" + "time" + + "github.com/apache/ossie/cli/internal/plugin" +) + +// TestMain handles two roles: +// 1. When GO_TEST_PLUGIN=1 it acts as a fake plugin subprocess, reads stdin, +// and writes a canned response based on GO_TEST_PLUGIN_MODE. +// 2. Otherwise it runs the normal test suite. +func TestMain(m *testing.M) { + if os.Getenv("GO_TEST_PLUGIN") == "1" { + runFakePlugin() + // runFakePlugin calls os.Exit; this line is never reached. + } + os.Exit(m.Run()) +} + +// runFakePlugin implements a minimal plugin subprocess used by invoke tests. +// It reads all of stdin (the JSON request), then dispatches on GO_TEST_PLUGIN_MODE. +func runFakePlugin() { + stdinBytes, err := io.ReadAll(os.Stdin) + if err != nil { + fmt.Fprintln(os.Stderr, "fake plugin: failed to read stdin:", err) + os.Exit(2) + } + + switch mode := os.Getenv("GO_TEST_PLUGIN_MODE"); mode { + case "success": + json.NewEncoder(os.Stdout).Encode(map[string]any{ + "files": map[string]string{"output.yaml": "converted content"}, + }) + + case "warning_issue": + json.NewEncoder(os.Stdout).Encode(map[string]any{ + "files": map[string]string{"output.yaml": "converted content"}, + "issues": []map[string]string{ + {"severity": "warning", "message": "some warning", "path": "input.yaml"}, + }, + }) + + case "error_issue": + json.NewEncoder(os.Stdout).Encode(map[string]any{ + "files": map[string]string{}, + "issues": []map[string]string{ + {"severity": "error", "message": "conversion failed"}, + }, + }) + + case "invalid_json": + fmt.Fprint(os.Stdout, "not json") + + case "stderr_output": + fmt.Fprint(os.Stderr, "stderr from plugin") + json.NewEncoder(os.Stdout).Encode(map[string]any{ + "files": map[string]string{"output.yaml": "ok"}, + }) + + case "nonzero_exit": + os.Exit(1) + + case "timeout": + time.Sleep(30 * time.Second) + + case "echo_request": + // Echo the raw stdin bytes back in files["received_request"] so the + // test can assert the plugin received the correct request JSON. + json.NewEncoder(os.Stdout).Encode(map[string]any{ + "files": map[string]string{"received_request": string(stdinBytes)}, + }) + + default: + fmt.Fprintln(os.Stderr, "fake plugin: unknown mode:", mode) + os.Exit(2) + } + + os.Exit(0) +} + +// fakePluginInvoke sets up env vars for the fake plugin and returns the +// invoke slice. The test binary re-invokes itself with -test.run=^$ so that +// no tests run in the child — TestMain sees GO_TEST_PLUGIN=1 and exits early. +func fakePluginInvoke(t *testing.T, mode string) []string { + t.Helper() + t.Setenv("GO_TEST_PLUGIN", "1") + t.Setenv("GO_TEST_PLUGIN_MODE", mode) + return []string{os.Args[0], "-test.run=^$"} +} + +func TestInvoke_success(t *testing.T) { + invoke := fakePluginInvoke(t, "success") + ctx := context.Background() + + resp, err := plugin.Invoke(ctx, t.TempDir(), invoke, plugin.Request{Files: map[string]string{}}, io.Discard) + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp == nil { + t.Fatal("expected non-nil response") + } + if _, ok := resp.Files["output.yaml"]; !ok { + t.Errorf("expected output.yaml in response files, got: %v", resp.Files) + } + if len(resp.Issues) != 0 { + t.Errorf("expected no issues, got %d", len(resp.Issues)) + } +} + +func TestInvoke_warningIssue(t *testing.T) { + invoke := fakePluginInvoke(t, "warning_issue") + ctx := context.Background() + + resp, err := plugin.Invoke(ctx, t.TempDir(), invoke, plugin.Request{}, io.Discard) + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(resp.Files) != 1 { + t.Errorf("expected 1 output file, got %d", len(resp.Files)) + } + if len(resp.Issues) != 1 { + t.Fatalf("expected 1 issue, got %d", len(resp.Issues)) + } + if resp.Issues[0].Severity != "warning" { + t.Errorf("issue severity: got %q, want %q", resp.Issues[0].Severity, "warning") + } +} + +func TestInvoke_errorIssue(t *testing.T) { + invoke := fakePluginInvoke(t, "error_issue") + ctx := context.Background() + + // An error-severity issue is NOT a Go error — Invoke must return nil err. + resp, err := plugin.Invoke(ctx, t.TempDir(), invoke, plugin.Request{}, io.Discard) + + if err != nil { + t.Fatalf("unexpected Go error: %v", err) + } + if len(resp.Issues) != 1 { + t.Fatalf("expected 1 issue, got %d", len(resp.Issues)) + } + if resp.Issues[0].Severity != "error" { + t.Errorf("issue severity: got %q, want %q", resp.Issues[0].Severity, "error") + } +} + +func TestInvoke_invalidResponseJSON(t *testing.T) { + invoke := fakePluginInvoke(t, "invalid_json") + ctx := context.Background() + + resp, err := plugin.Invoke(ctx, t.TempDir(), invoke, plugin.Request{}, io.Discard) + + if err == nil { + t.Fatal("expected error for invalid JSON response, got nil") + } + if resp != nil { + t.Errorf("expected nil response, got %+v", resp) + } +} + +func TestInvoke_nonZeroExit(t *testing.T) { + invoke := fakePluginInvoke(t, "nonzero_exit") + ctx := context.Background() + + resp, err := plugin.Invoke(ctx, t.TempDir(), invoke, plugin.Request{}, io.Discard) + + if err == nil { + t.Fatal("expected error for non-zero exit, got nil") + } + if resp != nil { + t.Errorf("expected nil response, got %+v", resp) + } +} + +func TestInvoke_stderrForwarded(t *testing.T) { + invoke := fakePluginInvoke(t, "stderr_output") + ctx := context.Background() + var buf strings.Builder + + _, err := plugin.Invoke(ctx, t.TempDir(), invoke, plugin.Request{}, &buf) + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !strings.Contains(buf.String(), "stderr from plugin") { + t.Errorf("expected stderr content in writer, got: %q", buf.String()) + } +} + +func TestInvoke_stderrSuppressed(t *testing.T) { + invoke := fakePluginInvoke(t, "stderr_output") + ctx := context.Background() + + // Passing io.Discard must not panic and must return a valid response. + resp, err := plugin.Invoke(ctx, t.TempDir(), invoke, plugin.Request{}, io.Discard) + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp == nil { + t.Fatal("expected non-nil response") + } +} + +func TestInvoke_timeout(t *testing.T) { + invoke := fakePluginInvoke(t, "timeout") + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + resp, err := plugin.Invoke(ctx, t.TempDir(), invoke, plugin.Request{}, io.Discard) + + if err == nil { + t.Fatal("expected timeout error, got nil") + } + if resp != nil { + t.Errorf("expected nil response on timeout, got %+v", resp) + } + if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("expected DeadlineExceeded in error chain, got: %v", err) + } +} + +func TestInvoke_requestPayloadReachesPlugin(t *testing.T) { + invoke := fakePluginInvoke(t, "echo_request") + ctx := context.Background() + + req := plugin.Request{ + Files: map[string]string{ + "core/orders.yaml": "version: 2\nmodels:\n - name: orders", + }, + } + + resp, err := plugin.Invoke(ctx, t.TempDir(), invoke, req, io.Discard) + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + raw, ok := resp.Files["received_request"] + if !ok { + t.Fatal("expected received_request key in response files") + } + + // Decode the echoed request and compare — avoids brittle JSON key-ordering checks. + var echoed plugin.Request + if err := json.Unmarshal([]byte(raw), &echoed); err != nil { + t.Fatalf("could not decode echoed request: %v", err) + } + want := req.Files["core/orders.yaml"] + if echoed.Files["core/orders.yaml"] != want { + t.Errorf("echoed file content:\ngot: %q\nwant: %q", echoed.Files["core/orders.yaml"], want) + } +} From 89e399b93dbc7a3d57d962d6614510514f1950b4 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Mon, 15 Jun 2026 13:48:54 -0500 Subject: [PATCH 2/3] fix(cli): normalise nil Request.Files to empty map before marshaling A Request with nil Files marshals as {"files":null} rather than {"files":{}}. Plugins that iterate over the files object may crash on a null value. Guard added at the top of Invoke(); test added to verify the wire format is always an object. --- cli/internal/plugin/invoke.go | 5 +++++ cli/internal/plugin/invoke_test.go | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/cli/internal/plugin/invoke.go b/cli/internal/plugin/invoke.go index d7e4018a..10f3b4a0 100644 --- a/cli/internal/plugin/invoke.go +++ b/cli/internal/plugin/invoke.go @@ -42,6 +42,11 @@ type Issue struct { // A non-empty Issues slice in the response is NOT itself a Go error — the // caller is responsible for inspecting severities and setting the exit code. func Invoke(ctx context.Context, pluginDir string, invoke []string, req Request, pluginStderr io.Writer) (*Response, error) { + // Normalise nil to empty map so plugins always receive {"files":{}} not {"files":null}. + if req.Files == nil { + req.Files = map[string]string{} + } + reqJSON, err := json.Marshal(req) if err != nil { return nil, fmt.Errorf("failed to marshal plugin request: %w", err) diff --git a/cli/internal/plugin/invoke_test.go b/cli/internal/plugin/invoke_test.go index 9345094b..a4404b8c 100644 --- a/cli/internal/plugin/invoke_test.go +++ b/cli/internal/plugin/invoke_test.go @@ -261,3 +261,21 @@ func TestInvoke_requestPayloadReachesPlugin(t *testing.T) { t.Errorf("echoed file content:\ngot: %q\nwant: %q", echoed.Files["core/orders.yaml"], want) } } + +func TestInvoke_nilFilesNormalisedToEmptyObject(t *testing.T) { + // A Request with nil Files must marshal as {"files":{}} not {"files":null}. + // Plugins receiving null instead of an object may crash on iteration. + invoke := fakePluginInvoke(t, "echo_request") + ctx := context.Background() + + resp, err := plugin.Invoke(ctx, t.TempDir(), invoke, plugin.Request{}, io.Discard) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + raw := resp.Files["received_request"] + + // The echoed bytes must contain `"files":{}` not `"files":null`. + if !strings.Contains(raw, `"files":{}`) { + t.Errorf("expected files to be serialised as {}, got: %s", raw) + } +} From 2deeacd60b58bb42aa9896cb19b7a2b9a0c92d47 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Mon, 15 Jun 2026 14:52:15 -0500 Subject: [PATCH 3/3] fix(cli): guard against empty invoke slice in Invoke An empty invoke slice would panic with an index out of range on invoke[0]. While F2 validation prevents this in normal usage, an explicit check with a descriptive error is safer and makes the failure mode obvious if the invariant is ever violated. --- cli/internal/plugin/invoke.go | 4 ++++ cli/internal/plugin/invoke_test.go | 13 +++++++++++++ 2 files changed, 17 insertions(+) diff --git a/cli/internal/plugin/invoke.go b/cli/internal/plugin/invoke.go index 10f3b4a0..17bf8480 100644 --- a/cli/internal/plugin/invoke.go +++ b/cli/internal/plugin/invoke.go @@ -42,6 +42,10 @@ type Issue struct { // A non-empty Issues slice in the response is NOT itself a Go error — the // caller is responsible for inspecting severities and setting the exit code. func Invoke(ctx context.Context, pluginDir string, invoke []string, req Request, pluginStderr io.Writer) (*Response, error) { + if len(invoke) == 0 { + return nil, fmt.Errorf("plugin invoke command is empty") + } + // Normalise nil to empty map so plugins always receive {"files":{}} not {"files":null}. if req.Files == nil { req.Files = map[string]string{} diff --git a/cli/internal/plugin/invoke_test.go b/cli/internal/plugin/invoke_test.go index a4404b8c..d3ca93ff 100644 --- a/cli/internal/plugin/invoke_test.go +++ b/cli/internal/plugin/invoke_test.go @@ -279,3 +279,16 @@ func TestInvoke_nilFilesNormalisedToEmptyObject(t *testing.T) { t.Errorf("expected files to be serialised as {}, got: %s", raw) } } + +func TestInvoke_emptyInvokeReturnsError(t *testing.T) { + ctx := context.Background() + + resp, err := plugin.Invoke(ctx, t.TempDir(), []string{}, plugin.Request{}, io.Discard) + + if err == nil { + t.Fatal("expected error for empty invoke slice, got nil") + } + if resp != nil { + t.Errorf("expected nil response, got %+v", resp) + } +}