feat: provide v2 plugin interface for pre eval init actions#64
feat: provide v2 plugin interface for pre eval init actions#64reecebedding wants to merge 8 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a protocol v2 runner plugin contract that adds a pre-evaluation Init step, plus automatic protocol-version detection for OCI-sourced plugins via image annotations.
Changes:
- Extend the runner gRPC/proto API with an
InitRPC and newInitRequest/InitResponsemessages. - Add a
RunnerV2plugin interface and versioned dispense names (runnervsrunner-v2), with agent-side selection based on config and OCI annotations. - Add tests for protocol defaulting/resolution and OCI manifest annotation parsing; document the approach in a new ADR.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| runner/proto/types.pb.go | Regenerated protobuf output (generator version bump). |
| runner/proto/runner_grpc.pb.go | Adds Init RPC client/server stubs and service descriptor entry. |
| runner/proto/runner.proto | Defines InitRequest/InitResponse and adds rpc Init(...). |
| runner/proto/runner.pb.go | Regenerated protobuf output reflecting Init* messages and service changes. |
| runner/proto/results.pb.go | Regenerated protobuf output (generator version bump). |
| runner/plugin.go | Introduces RunnerV2 interface; adds runner-v2 to plugin map. |
| runner/grpc_test.go | Adds test asserting v1 runners return Unimplemented for Init. |
| runner/grpc.go | Implements gRPC client/server support for Init and refactors API-helper server startup. |
| internal/oci_manifest_test.go | Adds unit tests for extracting annotations from OCI descriptors/manifests. |
| internal/oci.go | Adds GetAnnotations and manifest/descriptor annotation parsing logic. |
| docs/adr/0001-support-versioned-runner-plugins.md | ADR documenting versioned runner plugin support and OCI annotation behavior. |
| cmd/agent_test.go | Adds tests for protocol defaulting, resolution, and dispense name mapping. |
| cmd/agent.go | Adds protocol version config, OCI annotation-based resolution, v2 dispense name selection, and Init before Eval. |
| Makefile | Adds build/run targets. |
| .gitignore | Ignores .config directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var PluginMap = map[string]plugin.Plugin{ | ||
| "runner": &RunnerGRPCPlugin{}, | ||
| "runner": &RunnerGRPCPlugin{}, | ||
| "runner-v2": &RunnerGRPCPlugin{}, | ||
| } |
There was a problem hiding this comment.
Both runner and runner-v2 dispense names are mapped to the same RunnerGRPCPlugin implementation, and GRPCClient now always includes an Init method. That means a client dispensed under runner will still satisfy RunnerV2, making interface-based capability checks unreliable. Consider splitting into separate plugin/client types for v1 vs v2 (so only runner-v2 clients implement RunnerV2), or remove type assertions and instead treat codes.Unimplemented from Init as the signal that the plugin is not v2-capable.
| func updateAllPluginProtocols(agentConfig *agentConfig) { | ||
| for _, pluginConfig := range agentConfig.Plugins { | ||
| if pluginConfig.ProtocolVersion == 0 { | ||
| pluginConfig.ProtocolVersion = DefaultProtocolVersion | ||
| } | ||
| } |
There was a problem hiding this comment.
updateAllPluginProtocols defaults missing protocol versions but doesn't validate explicitly configured values (e.g., protocol_version: 3). Those will only fail later at runtime in runnerDispenseName, which can make misconfiguration harder to diagnose. Consider validating that ProtocolVersion is one of the supported values (1 or 2) during config merge/validation so the agent exits early with a clear config error.
| if pluginConfig.ProtocolVersion > 1 { | ||
| runnerV2, ok := runnerInstance.(runner.RunnerV2) | ||
| if !ok { | ||
| return fmt.Errorf("plugin %s configured as protocol_version=%d but does not support RunnerV2", pluginName, pluginConfig.ProtocolVersion) | ||
| } |
There was a problem hiding this comment.
The RunnerV2 type assertion here doesn't actually validate that the remote plugin supports protocol v2. The dispensed object is the host-side gRPC client stub, and it implements RunnerV2 unconditionally, so ok will always be true and this branch will never produce the intended error for a v1 plugin misconfigured as v2. Consider either (a) calling Init and converting a gRPC codes.Unimplemented response into a clearer "plugin does not support protocol v2" error, or (b) returning different client types from Dispense for runner vs runner-v2 so only v2 dispenses implement RunnerV2.
| if plugin.ProtocolVersion > 1 { | ||
| runnerV2, ok := runnerInstance.(runner.RunnerV2) | ||
| if !ok { | ||
| return fmt.Errorf("plugin %s configured as protocol_version=%d but does not support RunnerV2", name, plugin.ProtocolVersion) | ||
| } |
There was a problem hiding this comment.
Same issue as in runAllPlugins: the RunnerV2 type assertion will always succeed because the dispensed gRPC client stub implements Init regardless of the plugin's actual protocol support, so this check can't catch a v1 plugin configured as v2. Prefer checking for codes.Unimplemented from Init, or returning a v1 client type that does not implement RunnerV2 when dispensing runner.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if pluginConfig.ProtocolVersion == 0 { | ||
| pluginConfig.ProtocolVersion = DefaultProtocolVersion | ||
| } | ||
|
|
||
| if !isSupportedProtocolVersion(pluginConfig.ProtocolVersion) { | ||
| return fmt.Errorf("plugin %s has unsupported protocol_version=%d; supported values are %d and %d", name, pluginConfig.ProtocolVersion, DefaultProtocolVersion, RunnerV2ProtocolVersion) | ||
| } |
There was a problem hiding this comment.
Defaulting ProtocolVersion when it is 0 happens even when the user explicitly set protocol_version: 0 (you track explicitness via protocolSet). This silently turns an invalid explicit value into v1, and also prevents OCI annotation-based resolution from ever applying for that plugin. Consider only defaulting when protocolSet is false, and treating an explicit 0 as a validation error.
| func updateAllPluginProtocols(agentConfig *agentConfig) { | ||
| for _, pluginConfig := range agentConfig.Plugins { | ||
| if pluginConfig.ProtocolVersion == 0 { | ||
| pluginConfig.ProtocolVersion = DefaultProtocolVersion | ||
| } | ||
| } |
There was a problem hiding this comment.
updateAllPluginProtocols unconditionally defaults ProtocolVersion when it is 0. Since this runs after markExplicitPluginProtocols, it will also overwrite an explicitly configured protocol_version: 0 before validation can catch it. Consider guarding the defaulting with if !pluginConfig.protocolSet && pluginConfig.ProtocolVersion == 0 (and also nil-checking pluginConfig to be consistent with validation).
| type RunnerV2GRPCPlugin struct { | ||
| plugin.Plugin | ||
|
|
||
| // Impl Injection | ||
| Impl Runner | ||
| } |
There was a problem hiding this comment.
RunnerV2GRPCPlugin is registered under the runner-v2 dispense name, but its Impl field is typed as Runner rather than RunnerV2. This allows a v1 implementation to be wired into the v2 plugin entrypoint and only fail at runtime when Init is called. Consider changing Impl to RunnerV2 (and keeping GRPCServer.Impl as Runner) so v2 plugins must implement Init at compile time.
| for name, pluginConfig := range ac.Plugins { | ||
| if pluginConfig == nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
agentConfig.validate() currently ignores nil plugin entries (if pluginConfig == nil { continue }), but later code (downloads/runs) dereferences pluginConfig without nil checks. This can lead to a panic for configs like plugins: { foo: null }. Consider failing validation when a plugin config is null/missing instead of continuing.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for pluginName, pluginConfig := range ar.config.Plugins { | ||
| if pluginConfig == nil || pluginConfig.protocolSet || !internal.IsOCI(pluginConfig.Source) { | ||
| continue |
There was a problem hiding this comment.
resolvePluginProtocols() uses internal.IsOCI() to decide whether to fetch OCI annotations, but internal.IsOCI currently only recognizes tag references via name.NewTag(..., StrictValidation). OCI sources specified by digest (e.g., ghcr.io/repo@sha256:...) will be skipped and never get their protocol version resolved from annotations. Consider switching this check to a ParseReference-based predicate (or updating IsOCI) so both tag and digest references are treated as OCI sources.
cmd/agent.go
Outdated
| annotations, err := ar.fetchAnnotations(pluginConfig.Source) | ||
| if err != nil { | ||
| ar.logger.Warn("Failed to fetch plugin annotations, using configured/default protocol version", "plugin", pluginName, "source", pluginConfig.Source, "protocol_version", pluginConfig.ProtocolVersion, "error", err) | ||
| continue | ||
| } |
There was a problem hiding this comment.
resolvePluginProtocols() fetches remote OCI annotations without any cancellation/timeout (fetchAnnotations is called without remote.WithContext and GetAnnotations itself uses remote.Get with the default context). A slow or hung registry request can block agent startup and won't respect ctx cancellation. Consider threading ctx into resolvePluginProtocols/GetAnnotations and using remote.WithContext(ctx) and/or a bounded timeout.
| pluginExecutable, err := internal.Download(ctx, plugin.Source, AgentPluginDir, "plugin", ar.logger, remote.WithPlatform(v1.Platform{ | ||
| Architecture: runtime.GOARCH, | ||
| OS: runtime.GOOS, | ||
| })) | ||
|
|
||
| fmt.Println("Running plugin", "source", plugin.Source) | ||
| fmt.Println("Running plugin", "source", pluginExecutable) | ||
| ar.logger.Info("Running plugin", "source", plugin.Source, "protocol_version", plugin.ProtocolVersion) | ||
| ar.logger.Info("Running plugin", "source", pluginExecutable, "protocol_version", plugin.ProtocolVersion) | ||
|
|
||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
runPlugin logs the downloaded pluginExecutable path before checking the error returned by internal.Download(). If Download fails, this logs an empty/partial path and loses the opportunity to log the failure context. Consider moving these log lines after the err check (or including the error in the log when err != nil).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func GetAnnotations(ctx context.Context, source string, option ...remote.Option) (map[string]string, error) { | ||
| ref, err := name.ParseReference(source) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
GetAnnotations uses name.ParseReference(source) without name.StrictValidation, while IsOCI uses strict validation. For consistency (and to avoid accepting references that IsOCI would reject if GetAnnotations is called elsewhere), consider parsing with name.ParseReference(source, name.StrictValidation) here as well.
| func IsOCI(source string) bool { | ||
| // Check whether this can be parsed as an OCI endpoint | ||
| _, err := name.NewTag(source, name.StrictValidation) | ||
| _, err := name.ParseReference(source, name.StrictValidation) | ||
| return err == nil | ||
| } |
There was a problem hiding this comment.
IsOCI now accepts digest references (e.g. repo/image@sha256:...) via name.ParseReference, but the download path in this file still parses OCI sources with name.NewTag(...), which will fail for digests. This makes digest sources appear supported while downloads will error. Either add digest-support to the downloader path (parse with name.ParseReference and handle both tags and digests) or keep IsOCI tag-only.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
runner/grpc.go
Outdated
| brokerID := m.broker.NextId() | ||
| go m.broker.AcceptAndServe(brokerID, serverFunc) | ||
|
|
||
| request.ApiServer = brokerID | ||
| return brokerID |
There was a problem hiding this comment.
startAPIServer launches broker.AcceptAndServe in a goroutine but never shuts down the gRPC server / broker entry afterward. Since Eval (and now Init) call this per request, repeated plugin runs (e.g., cron/daemon mode) can leak goroutines/listeners over time. Consider returning a cleanup function from startAPIServer (or otherwise ensuring the server is stopped and the broker ID released) once the RPC completes, and invoke it in both Eval and Init paths.
cmd/agent.go
Outdated
| } | ||
|
|
||
| for pluginName, pluginConfig := range ar.config.Plugins { | ||
| if pluginConfig == nil || pluginConfig.protocolSet || !internal.IsOCIReference(pluginConfig.Source) { |
There was a problem hiding this comment.
resolvePluginProtocols uses internal.IsOCIReference, which includes digest references. But downloads are still gated by internal.IsOCI (tag-only), so a digest-based plugin source can reach this annotation lookup path but later fail during plugin download/execution. To avoid this inconsistent behavior, align the predicate with what is actually supported (use the same OCI check as the downloader), or update the downloader to support digest references as OCI sources.
| if pluginConfig == nil || pluginConfig.protocolSet || !internal.IsOCIReference(pluginConfig.Source) { | |
| if pluginConfig == nil || pluginConfig.protocolSet || !internal.IsOCI(pluginConfig.Source) { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for name, pluginConfig := range ac.Plugins { | ||
| if pluginConfig == nil { | ||
| return fmt.Errorf("plugin %s has null configuration", name) | ||
| } | ||
|
|
||
| if pluginConfig.ProtocolVersion == 0 { | ||
| if pluginConfig.protocolSet { | ||
| return fmt.Errorf("plugin %s has unsupported protocol_version=%d; supported values are %d and %d", name, pluginConfig.ProtocolVersion, DefaultProtocolVersion, RunnerV2ProtocolVersion) | ||
| } | ||
|
|
||
| pluginConfig.ProtocolVersion = DefaultProtocolVersion | ||
| } | ||
|
|
||
| if !isSupportedProtocolVersion(pluginConfig.ProtocolVersion) { | ||
| return fmt.Errorf("plugin %s has unsupported protocol_version=%d; supported values are %d and %d", name, pluginConfig.ProtocolVersion, DefaultProtocolVersion, RunnerV2ProtocolVersion) | ||
| } | ||
| } |
There was a problem hiding this comment.
(*agentConfig).validate() is mutating configuration (pluginConfig.ProtocolVersion = DefaultProtocolVersion). This makes validation have side effects and duplicates the defaulting logic already performed in updateAllPluginProtocols(), which can lead to confusing call-order dependencies. Consider moving all defaulting into a single place (e.g., mergeConfig / updateAllPluginProtocols) and making validate() purely check invariants without modifying state.
cmd/agent.go
Outdated
| annotationCtx, cancel := context.WithTimeout(ctx, 30*time.Second) | ||
| annotations, err := ar.fetchAnnotations(annotationCtx, pluginConfig.Source) | ||
| cancel() | ||
| if err != nil { | ||
| ar.logger.Warn("Failed to fetch plugin annotations, using configured/default protocol version", "plugin", pluginName, "source", pluginConfig.Source, "protocol_version", pluginConfig.ProtocolVersion, "error", err) | ||
| continue | ||
| } |
There was a problem hiding this comment.
The cancel() should be deferred immediately after context.WithTimeout to ensure resources are released even if fetchAnnotations panics or future edits introduce early returns. Suggested pattern: call annotationCtx, cancel := ...; defer cancel() within the loop iteration.
internal/oci.go
Outdated
| return payload.Annotations | ||
| } | ||
| } | ||
|
|
||
| if len(desc.Annotations) > 0 { | ||
| return desc.Annotations |
There was a problem hiding this comment.
annotationsFromDescriptor sometimes returns the underlying desc.Annotations map directly. Returning shared maps can cause unexpected aliasing if callers modify the returned map. Prefer returning a defensive copy for both manifest-derived annotations and descriptor-derived annotations (and keep the empty-map case as a new map) to make the function safe to use.
| return payload.Annotations | |
| } | |
| } | |
| if len(desc.Annotations) > 0 { | |
| return desc.Annotations | |
| copied := make(map[string]string, len(payload.Annotations)) | |
| for k, v := range payload.Annotations { | |
| copied[k] = v | |
| } | |
| return copied | |
| } | |
| } | |
| if len(desc.Annotations) > 0 { | |
| copied := make(map[string]string, len(desc.Annotations)) | |
| for k, v := range desc.Annotations { | |
| copied[k] = v | |
| } | |
| return copied |
cmd/agent.go
Outdated
|
|
||
| // Request the plugin | ||
| raw, err := rpcClient.Dispense("runner") | ||
| logger.Debug("Dispensing plugin", "runner", dispenseName) |
There was a problem hiding this comment.
The log field key runner is ambiguous here (it holds the dispense name, not the runner instance). Consider renaming the key to something like dispense_name (or plugin_name) to make logs easier to interpret.
| logger.Debug("Dispensing plugin", "runner", dispenseName) | |
| logger.Debug("Dispensing plugin", "dispense_name", dispenseName) |
|
|
||
| - adding `protocol_version` to plugin configuration | ||
| - defaulting unspecified plugins to protocol version 1 | ||
| - reading `org.ccf.plugin.protocol.version` from OCI annotations for OCI plugin sources without an explicit `protocol_version` |
There was a problem hiding this comment.
internal.IsOCI now explicitly treats only OCI tag references as supported (digest references are not considered OCI by that check). The ADR currently says 'OCI plugin sources' broadly; consider clarifying that annotation lookup (and download) applies to tag-form references (e.g., repo:tag) and not digest-form references (e.g., repo@sha256:...) to avoid confusion.
| - reading `org.ccf.plugin.protocol.version` from OCI annotations for OCI plugin sources without an explicit `protocol_version` | |
| - reading `org.ccf.plugin.protocol.version` from OCI annotations for OCI plugin sources referenced by tag (for example, `repo:tag`) without an explicit `protocol_version` (digest-form references, such as `repo@sha256:...`, do not use OCI annotation lookup) |
internal/utils_test.go
Outdated
| expected: true, | ||
| }, | ||
| { | ||
| name: "Basic OCI url with digest", |
There was a problem hiding this comment.
Test name reads like it should be accepted as an OCI reference, but the expectation is false (which matches the updated semantics: only tag refs are supported). Consider renaming to reflect intent, e.g. 'Digest OCI references are not treated as supported OCI tags'.
| name: "Basic OCI url with digest", | |
| name: "Digest OCI reference is not treated as supported OCI tag", |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
runner/grpc.go
Outdated
| @@ -41,27 +60,47 @@ func (m *GRPCApiHelperServer) CreateEvidence(ctx context.Context, req *proto.Cre | |||
| type GRPCClient struct { | |||
There was a problem hiding this comment.
The comment "GRPCClient is an implementation of KV" appears to be leftover copy/paste and doesn’t match the Runner/ApiHelper responsibilities in this file, which can mislead future readers. Update it to describe that GRPCClient implements the Runner (and GRPCClientV2 implements RunnerV2) over go-plugin gRPC.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.