feat(cli): add plugin invocation protocol#155
Merged
Conversation
This was referenced Jun 15, 2026
Contributor
Author
QMalcolm
force-pushed
the
qmalcolm--feat-plugin-discovery
branch
from
June 23, 2026 21:05
9b858f5 to
9811e05
Compare
QMalcolm
force-pushed
the
qmalcolm--feat-plugin-io-protocol
branch
from
June 23, 2026 22:06
37a0bc7 to
fce4b4d
Compare
QMalcolm
marked this pull request as ready for review
June 23, 2026 22:22
QMalcolm
force-pushed
the
qmalcolm--feat-plugin-discovery
branch
from
June 24, 2026 21:30
4cdc89d to
bca8161
Compare
QMalcolm
force-pushed
the
qmalcolm--feat-plugin-io-protocol
branch
from
June 24, 2026 21:31
fce4b4d to
fdecf45
Compare
This was referenced Jul 8, 2026
QMalcolm
force-pushed
the
qmalcolm--feat-plugin-discovery
branch
from
July 17, 2026 21:38
bca8161 to
f2873b5
Compare
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
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.
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.
QMalcolm
force-pushed
the
qmalcolm--feat-plugin-io-protocol
branch
from
July 22, 2026 18:01
fdecf45 to
2deeacd
Compare
|
|
||
| var stdout bytes.Buffer | ||
| cmd := exec.CommandContext(ctx, invoke[0], invoke[1:]...) | ||
| cmd.Dir = pluginDir |
Contributor
There was a problem hiding this comment.
Would this fail nicely if pluginDir is not accessible ? Would be good to add error handling for this case.
Contributor
Author
There was a problem hiding this comment.
Currently, when the pluginDir is not accessible the failure looks like
plugin process failed: chdir /nonexistent/path/xyz: no such file or directoryAdditionally it's gets wrapped in a normal Go error instead of becoming a panic. Considering that, I think we can say it fails gracefully 🙂
Contributor
|
LGTM, but some good follow ups for future for anyone interested:
Should be we track these as Issues ? |
khush-bhatia
approved these changes
Jul 22, 2026
Contributor
Author
I definitely think we should track (1) as an issue. I'm not sure exactly how we want to do this long term. Having the CLI handle read/write means that each plugin doesn't need to implement handing that with the OS, and ensures that glob based file lookup is always handled the same regardless of plugin used. However for sufficiently large semantic layers, it will probably be meaningfully impactful if the plugins handle file system read/write themselves. For item (2), if we have specific areas (like protocol spec for query API), those would probably be good to create issues to track 🙂 |
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
2 participants
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
Request,Response, andIssueJSON envelope types for the plugin stdin/stdout protocolInvoke(ctx, pluginDir, invoke, req, pluginStderr)— runs a plugin subprocess, pipes the request JSON to stdin, captures stdout as the response, and enforces a context timeoutIssuesslice in the response is not a Go error; the caller (defined in a later segment of work) will decide whether error-severity issues produce a non-zero exit code