From 8bf07f1345daedb3a3521058d58ae92a5b125dd8 Mon Sep 17 00:00:00 2001 From: Ryan Tinianov Date: Thu, 15 Oct 2026 15:01:16 -0400 Subject: [PATCH 1/2] Move to dependency injection for PSBs --- libs/capability/capability.go | 40 ------ libs/capability/run.go | 137 +------------------- libs/capability/run_test.go | 209 +++++------------------------- libs/capability/runner.go | 19 ++- libs/capability/runner_test.go | 38 +++--- libs/capability/telemetry_test.go | 4 +- libs/go.mod | 2 + libs/go.sum | 4 + 8 files changed, 72 insertions(+), 381 deletions(-) diff --git a/libs/capability/capability.go b/libs/capability/capability.go index ab7cf27a4..2474c2c6d 100644 --- a/libs/capability/capability.go +++ b/libs/capability/capability.go @@ -1,15 +1,10 @@ package capability import ( - "reflect" - "google.golang.org/protobuf/reflect/protoreflect" "github.com/smartcontractkit/chainlink-common/pkg/capabilities" - "github.com/smartcontractkit/chainlink-common/pkg/logger" "github.com/smartcontractkit/chainlink-common/pkg/services" - "github.com/smartcontractkit/chainlink-common/pkg/settings/limits" - "github.com/smartcontractkit/chainlink-common/pkg/types/core" ) type Capability interface { @@ -20,19 +15,6 @@ type Capability interface { Service() protoreflect.ServiceDescriptor } -// Config marks the struct a constructor asks for as the capability's own configuration -// -// type Config struct { -// capability.Config -// FastestScheduleIntervalSeconds int `usage:"fastest cron schedule a workflow may register"` -// } -// -// The run binds the struct's fields as flags on the root command, namespaced by the binary's -// name - cron's are --cron.fastest-schedule-interval-seconds, CRE_CRON_FASTEST_SCHEDULE_INTERVAL_SECONDS, -// or the same key in the config file - and hands the constructor the decoded value. A constructor -// declares at most one. -type Config struct{} - // capabilityConfig is what a capability binary needs from the node it runs beside. type capabilityConfig struct { ProxyURL string `usage:"gRPC target of the node's capability registry proxy (e.g. localhost:9000), used to resolve capabilities this binary does not host. Unset resolves only the capabilities this binary hosts"` @@ -41,25 +23,3 @@ type capabilityConfig struct { // Serves the Debug UI HTTPDebug bool `usage:"serve the capability debug UI on the shared HTTP server, under /debug/capabilities"` } - -type Dependencies struct { - Logger logger.Logger - CapabilityRegistry core.CapabilitiesRegistry - LimitsFactory limits.Factory -} - -func (d Dependencies) list() []any { - return []any{d.Logger, d.CapabilityRegistry, d.LimitsFactory} -} - -func (d Dependencies) resolve(want reflect.Type) (reflect.Value, bool) { - for _, v := range d.list() { - if v == nil { - continue - } - if got := reflect.TypeOf(v); got.AssignableTo(want) { - return reflect.ValueOf(v), true - } - } - return reflect.Value{}, false -} diff --git a/libs/capability/run.go b/libs/capability/run.go index 8fed0efa4..3e7a12bea 100644 --- a/libs/capability/run.go +++ b/libs/capability/run.go @@ -3,14 +3,13 @@ package capability import ( "context" "errors" - "fmt" "log" "os" "os/signal" "path/filepath" - "reflect" "syscall" + "github.com/samber/do/v2" "github.com/spf13/cobra" sdkmetric "go.opentelemetry.io/otel/sdk/metric" @@ -21,7 +20,7 @@ import ( // process is signalled. // // func main() { capability.Run(context.Background(), trigger.NewCron) } -func Run(ctx context.Context, constructor any, opts ...Option) { +func Run(ctx context.Context, constructor func(do.Injector) (Capability, error), opts ...Option) { lggr, err := NewLogger() if err != nil { log.Fatal(err) @@ -36,17 +35,11 @@ func Run(ctx context.Context, constructor any, opts ...Option) { // // A run blocks until it is told to stop, which is either ctx being cancelled or this process being // signalled - see below. -func RunErr(ctx context.Context, lggr logger.Logger, constructor any, opts ...Option) error { +func RunErr(ctx context.Context, lggr logger.Logger, constructor func(do.Injector) (Capability, error), opts ...Option) error { if lggr == nil { return errors.New("must provide a logger") } - // We instantiate this up-front so we can bubble up an error message about an invalid `constructor` early. - c, err := newConstructor(constructor) - if err != nil { - return err - } - // TODO: Run should accept some kind of Info struct root := &cobra.Command{ Use: filepath.Base(os.Args[0]), @@ -64,19 +57,12 @@ func RunErr(ctx context.Context, lggr logger.Logger, constructor any, opts ...Op return err } - // Bind the capability's config if the constructor function declares a config struct. - // This will be namespaced using the name of the root command. - // eg. --cron.fastest-schedule-interval-seconds - if err := c.bindCapabilityConfig(root); err != nil { - return err - } - root.AddCommand(&cobra.Command{ Use: "run", Short: "Run the capability", RunE: func(cmd *cobra.Command, _ []string) error { // TODO: pass in constructors here for our runnable/embeddable dependencies? - return run(cmd.Context(), lggr, cmd.Root().Name(), cfg, c) + return run(cmd.Context(), lggr, cmd.Root().Name(), cfg, constructor) }, }) @@ -87,7 +73,7 @@ func RunErr(ctx context.Context, lggr logger.Logger, constructor any, opts ...Op RunE: func(cmd *cobra.Command, _ []string) error { // TODO: pass in constructors here for our runnable/embeddable dependencies? // Something else here - return run(cmd.Context(), lggr, cmd.Root().Name(), cfg, c) + return run(cmd.Context(), lggr, cmd.Root().Name(), cfg, constructor) }, }) @@ -98,119 +84,6 @@ func RunErr(ctx context.Context, lggr logger.Logger, constructor any, opts ...Op return root.ExecuteContext(ctx) } -type constructorFn struct { - fn reflect.Value - typ reflect.Type - - // configIn is the index of the parameter that is the capability's own config, or -1 when the - // constructor declares none. - configIn int - // config is the instance of that parameter the flags decode fills. Kept here rather than - // passed to run because the decode hook fills it in place when the command runs: call has to - // read that same memory, not a copy made when it was bound. Valid only when configIn >= 0. - config reflect.Value -} - -var capabilityType = reflect.TypeFor[Capability]() - -// newConstructor validates that constructor has the right shape, i.e.: -// - it's a function -// - and the function returns a capability and optionally an error -func newConstructor(ctor any) (constructorFn, error) { - t := reflect.TypeOf(ctor) - if t == nil || t.Kind() != reflect.Func { - return constructorFn{}, fmt.Errorf("a capability constructor must be a function, got %T", ctor) - } - - switch t.NumOut() { - case 1: - case 2: - if t.Out(1) != reflect.TypeFor[error]() { - return constructorFn{}, fmt.Errorf( - "a capability constructor returning two values must return an error second, got %s", t) - } - default: - return constructorFn{}, fmt.Errorf( - "a capability constructor must return the capability, and optionally an error, got %s", t) - } - - if out := t.Out(0); !out.Implements(capabilityType) { - return constructorFn{}, fmt.Errorf("%s does not implement capability.Capability", out) - } - - configIn := -1 - for i := range t.NumIn() { - if !isConfig(t.In(i)) { - continue - } - if configIn >= 0 { - return constructorFn{}, fmt.Errorf("a capability constructor takes its config once, got %s", t) - } - configIn = i - } - return constructorFn{fn: reflect.ValueOf(ctor), typ: t, configIn: configIn}, nil -} - -var isConfigType = reflect.TypeFor[Config]() - -// isConfig reports whether t declares itself a capability's config by embedding Config. The -// embedding has to be direct: the marker declares the struct it is embedded in, not the structs -// that embed that one. -func isConfig(t reflect.Type) bool { - if t.Kind() != reflect.Struct { - return false - } - for i := range t.NumField() { - if f := t.Field(i); f.Anonymous && f.Type == isConfigType { - return true - } - } - return false -} - -// bindCapabilityConfig creates the config the constructor declares, if any, and binds its fields as flags -// on root, under root's name. -func (c *constructorFn) bindCapabilityConfig(root *cobra.Command) error { - if c.configIn < 0 { - return nil - } - - v := reflect.New(c.typ.In(c.configIn)) - if err := bindFlags(root, root.Name(), v.Interface()); err != nil { - return fmt.Errorf("failed to register the capability's config: %w", err) - } - c.config = v.Elem() - return nil -} - -// call builds the capability by calling the contstructor. -// The constructor's parameters are scanned, and type matched against the dependency set. -// The config parameter is treated exceptionally and hydrated from the configuration that was passed in. -func (c constructorFn) call(deps Dependencies) (Capability, error) { - args := make([]reflect.Value, c.typ.NumIn()) - for i := range args { - if i == c.configIn { - args[i] = c.config - continue - } - - want := c.typ.In(i) - - v, ok := deps.resolve(want) - if !ok { - return nil, fmt.Errorf("the capability constructor asks for a %s, and nothing in this run "+ - "provides one: %s", want, c.typ) - } - args[i] = v - } - - out := c.fn.Call(args) - if len(out) == 2 && !out[1].IsNil() { - return nil, fmt.Errorf("failed to build the capability: %w", out[1].Interface().(error)) - } - return out[0].Interface().(Capability), nil -} - type Option func(*config) // WithOtelViews sets otel metric views - histogram bucket boundaries, typically - on the beholder diff --git a/libs/capability/run_test.go b/libs/capability/run_test.go index ea97349d7..2bb708dea 100644 --- a/libs/capability/run_test.go +++ b/libs/capability/run_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + "github.com/samber/do/v2" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/protobuf/reflect/protoreflect" @@ -80,7 +81,7 @@ func newFake() *fake { // A run blocks until its context is cancelled, so this waits until the binary is serving and then // cancels - which is a test standing in for the signal an operator would send. A run that fails on // the way up never gets there and reports that instead. -func execute(t *testing.T, lggr logger.Logger, ctor any, args ...string) error { +func execute(t *testing.T, lggr logger.Logger, ctor func(do.Injector) (Capability, error), args ...string) error { t.Helper() // A free port rather than the default, since tests share a machine and run alongside each @@ -154,212 +155,61 @@ func freePort(t *testing.T) int { return port } -func TestRunErrRejects(t *testing.T) { - lggr := logger.Test(t) +// fakeConstructor builds a fake and asks the injector for nothing. +func fakeConstructor(do.Injector) (Capability, error) { return newFake(), nil } +func TestRunErrRejects(t *testing.T) { t.Run("no logger", func(t *testing.T) { - require.ErrorContains(t, RunErr(context.Background(), nil, newFake), "must provide a logger") - }) - - t.Run("something that is not a function", func(t *testing.T) { - require.ErrorContains(t, RunErr(context.Background(), lggr, "cron"), "must be a function, got string") - }) - - t.Run("nothing at all", func(t *testing.T) { - require.ErrorContains(t, RunErr(context.Background(), lggr, nil), "must be a function") - }) - - t.Run("a function returning something that cannot be hosted", func(t *testing.T) { - require.ErrorContains(t, RunErr(context.Background(), lggr, func() string { return "" }), - "string does not implement capability.Capability") - }) - - t.Run("a function returning nothing", func(t *testing.T) { - require.ErrorContains(t, RunErr(context.Background(), lggr, func() {}), "must return the capability") - }) - - t.Run("a function whose second result is not an error", func(t *testing.T) { - require.ErrorContains(t, RunErr(context.Background(), lggr, func() (*fake, string) { return nil, "" }), - "must return an error second") + require.ErrorContains(t, RunErr(context.Background(), nil, fakeConstructor), "must provide a logger") }) } func TestRunBuildsTheCapability(t *testing.T) { built := 0 - require.NoError(t, execute(t, logger.Test(t), func() *fake { built++; return newFake() }, "run")) + require.NoError(t, execute(t, logger.Test(t), func(do.Injector) (Capability, error) { + built++ + return newFake(), nil + }, "run")) assert.Equal(t, 1, built, "the constructor should have been called exactly once") } func TestRunReportsAFailedConstructor(t *testing.T) { - err := execute(t, logger.Test(t), func() (*fake, error) { return nil, errors.New("no schedule") }, "run") - require.ErrorContains(t, err, "failed to build the capability: no schedule") -} - -// TestRunNamesWhatItCannotProvide covers a parameter nothing answers: it is reported before -// anything starts, and names the type that went unmatched rather than the position it was in. -func TestRunNamesWhatItCannotProvide(t *testing.T) { - err := execute(t, logger.Test(t), func(string) *fake { return newFake() }, "run") - require.ErrorContains(t, err, "asks for a string, and nothing in this run provides one") + err := execute(t, logger.Test(t), func(do.Injector) (Capability, error) { + return nil, errors.New("no schedule") + }, "run") + require.ErrorContains(t, err, "failed to instantiate capability: no schedule") } -// TestRunPassesTheLoggerToTheConstructor covers the first dependency: this process's logger, so a -// capability names its own from it rather than building one the operator cannot configure. -func TestRunPassesTheLoggerToTheConstructor(t *testing.T) { - var got logger.Logger - - require.NoError(t, execute(t, logger.Test(t), func(l logger.Logger) *fake { - got = l - return newFake() - }, "run")) - - assert.NotNil(t, got, "the constructor should have been handed the run's logger") -} - -// TestRunPassesTheRegistryToTheConstructor covers the registry a capability resolves other -// capabilities through. -// -// By type rather than by position, so a constructor asks for what it needs and takes nothing it -// does not - which is what lets the one below coexist with the no-argument constructors elsewhere -// in these tests. -func TestRunPassesTheRegistryToTheConstructor(t *testing.T) { - var got core.CapabilitiesRegistry - - require.NoError(t, execute(t, logger.Test(t), func(r core.CapabilitiesRegistry) *fake { - got = r - return newFake() - }, "run")) - - assert.NotNil(t, got, "the constructor should have been handed the run's registry") -} - -// TestRunPassesTheLimitsFactoryToTheConstructor covers the second dependency: what a capability -// bounds a workflow's requests with. -// -// A struct rather than an interface, which the type matching handles the same way - what a -// constructor asks for is a type, not a kind of type. -func TestRunPassesTheLimitsFactoryToTheConstructor(t *testing.T) { - var got limits.Factory - - require.NoError(t, execute(t, logger.Test(t), func(f limits.Factory) *fake { - got = f - return newFake() - }, "run")) - - assert.NotNil(t, got.Settings, "the factory should be built over this run's settings") - assert.NotNil(t, got.Meter) -} - -// TestRunPassesEveryDependencyItHas covers a constructor asking for more than one, in an order of -// its own: matching is by type, so the order it lists them in is its business. -func TestRunPassesEveryDependencyItHas(t *testing.T) { +// TestRunProvidesItsDependencies covers what the injector holds: this process's logger, the +// registry capabilities are resolved through, and the factory a workflow's requests are bounded by. +func TestRunProvidesItsDependencies(t *testing.T) { var ( - gotLimits limits.Factory + gotLogger logger.Logger gotRegistry core.CapabilitiesRegistry + gotLimits limits.Factory ) - require.NoError(t, execute(t, logger.Test(t), func(f limits.Factory, r core.CapabilitiesRegistry) *fake { - gotLimits, gotRegistry = f, r - return newFake() + require.NoError(t, execute(t, logger.Test(t), func(i do.Injector) (Capability, error) { + gotLogger = do.MustInvoke[logger.Logger](i) + gotRegistry = do.MustInvoke[core.CapabilitiesRegistry](i) + gotLimits = do.MustInvoke[limits.Factory](i) + return newFake(), nil }, "run")) - assert.NotNil(t, gotLimits.Settings) + assert.NotNil(t, gotLogger) assert.NotNil(t, gotRegistry) -} - -// testConfig is a capability's own config, declared by embedding Config: the one parameter an -// operator supplies rather than the run resolves. -type testConfig struct { - Config - Shout string `usage:"what the fake says"` -} - -// TestRunHandsTheConstructorItsConfig covers the parameter that is not a dependency: the -// capability's own settings, bound under the binary's name and handed over decoded. The binary is -// named cron - execute's argv[0] - so its flag is --cron.shout. -func TestRunHandsTheConstructorItsConfig(t *testing.T) { - var got testConfig - - require.NoError(t, execute(t, logger.Test(t), func(cfg testConfig) *fake { - got = cfg - return newFake() - }, "run", "--cron.shout", "hello")) - - assert.Equal(t, "hello", got.Shout) -} - -// TestRunReadsTheCapabilityConfigFromTheEnvironment is the same binding from the other direction: -// CRE_, then the binary's name, as every other section's settings are. -func TestRunReadsTheCapabilityConfigFromTheEnvironment(t *testing.T) { - t.Setenv("CRE_CRON_SHOUT", "from the environment") - - var got testConfig - - require.NoError(t, execute(t, logger.Test(t), func(cfg testConfig) *fake { - got = cfg - return newFake() - }, "run")) - - assert.Equal(t, "from the environment", got.Shout) -} - -// TestRunHandsTheConstructorItsConfigAndItsDependencies covers the two kinds of parameter side by -// side: the config from the flags, the dependencies from the run, and neither answered by the -// other. -func TestRunHandsTheConstructorItsConfigAndItsDependencies(t *testing.T) { - var ( - gotCfg testConfig - gotLimits limits.Factory - ) - - require.NoError(t, execute(t, logger.Test(t), func(cfg testConfig, f limits.Factory) *fake { - gotCfg, gotLimits = cfg, f - return newFake() - }, "run", "--cron.shout", "hello")) - - assert.Equal(t, "hello", gotCfg.Shout) assert.NotNil(t, gotLimits.Settings) -} - -// TestRunRejectsTwoConfigs pins the one-config rule: the struct is bound under the binary's name, -// so two of them would share one namespace and which setting belonged to which would be anyone's -// guess. -func TestRunRejectsTwoConfigs(t *testing.T) { - err := execute(t, logger.Test(t), func(a, b testConfig) *fake { return newFake() }, "run") - require.ErrorContains(t, err, "takes its config once") -} - -// TestRunRejectsTheDependenciesStruct pins the rule the other way round: a constructor declares the -// things it uses, not the bag they came in. -// -// Taking the struct would be a dependency on everything a run has, and adding a field to it would -// silently widen what every capability appeared to need. -func TestRunRejectsTheDependenciesStruct(t *testing.T) { - err := execute(t, logger.Test(t), func(Dependencies) *fake { return newFake() }, "run") - - require.ErrorContains(t, err, "asks for a capability.Dependencies, and nothing in this run provides one") -} - -// TestRunPassesTheRegistryAsANarrowerInterface covers matching by assignability: a capability that -// only needs to resolve capabilities can say so, without naming the wider type the run holds. -func TestRunPassesTheRegistryAsANarrowerInterface(t *testing.T) { - var got core.CapabilitiesRegistry - - require.NoError(t, execute(t, logger.Test(t), func(r core.CapabilitiesRegistry) *fake { - got = r - return newFake() - }, "run")) - - assert.NotNil(t, got) + assert.NotNil(t, gotLimits.Meter) } // TestRunHasARunCommand is the shape of the binary rather than what it does: a root that runs // nothing itself, and a way to start it hanging off it. func TestRunHasARunCommand(t *testing.T) { // A bare invocation prints help and does nothing, rather than starting the capability. - require.NoError(t, execute(t, logger.Test(t), func() *fake { + require.NoError(t, execute(t, logger.Test(t), func(do.Injector) (Capability, error) { t.Error("the root command should not build the capability") - return newFake() + return newFake(), nil })) } @@ -372,8 +222,7 @@ func TestRunServesAndAnnouncesTheCapability(t *testing.T) { node := newSpyRegistry(t) proxyURL := serveNodeRegistry(t, node) - require.NoError(t, execute(t, logger.Test(t), func() *fake { return newFake() }, - "run", "--capabilities.proxy-url", proxyURL)) + require.NoError(t, execute(t, logger.Test(t), fakeConstructor, "run", "--capabilities.proxy-url", proxyURL)) assert.Equal(t, []string{fakeID}, node.addedIDs(), "the run should have announced the capability to the node's registry") assert.Equal(t, []string{fakeID}, node.removedIDs(), "and deregistered it on the way out") diff --git a/libs/capability/runner.go b/libs/capability/runner.go index e3a3001e2..caf212c0d 100644 --- a/libs/capability/runner.go +++ b/libs/capability/runner.go @@ -7,14 +7,16 @@ import ( "os" "github.com/hashicorp/go-plugin" + "github.com/samber/do/v2" "github.com/smartcontractkit/chainlink-common/pkg/beholder" "github.com/smartcontractkit/chainlink-common/pkg/logger" "github.com/smartcontractkit/chainlink-common/pkg/loop" "github.com/smartcontractkit/chainlink-common/pkg/services" + "github.com/smartcontractkit/chainlink-common/pkg/types/core" ) -func run(ctx context.Context, lggr logger.Logger, name string, cfg *config, newCapability constructorFn) error { +func run(ctx context.Context, lggr logger.Logger, name string, cfg *config, newCapability func(do.Injector) (Capability, error)) error { defer func() { _ = lggr.Sync() }() var svcs []services.Service @@ -105,12 +107,15 @@ func underPluginHost() bool { // startCapability builds the capability from its constructor and makes it reachable: started, // served and announced by the registry, and mounted via the debug UI if debug mode is turned on. -func startCapability(ctx context.Context, lggr logger.Logger, cfg *config, ctor constructorFn, reg *registryService, settings *loop.AtomicSettings, mux *http.ServeMux) (Capability, error) { - c, err := ctor.call(Dependencies{ - Logger: lggr, - CapabilityRegistry: reg, - LimitsFactory: newLimitsFactory(lggr, settings), - }) +func startCapability(ctx context.Context, lggr logger.Logger, cfg *config, ctor func(do.Injector) (Capability, error), reg *registryService, settings *loop.AtomicSettings, mux *http.ServeMux) (Capability, error) { + // The run supervises the services it provides, so the injector is never shut down: doing so + // would close them a second time. + i := do.New() + do.ProvideValue(i, lggr) + do.ProvideValue[core.CapabilitiesRegistry](i, reg) + do.ProvideValue(i, newLimitsFactory(lggr, settings)) + + c, err := ctor(i) if err != nil { return nil, fmt.Errorf("failed to instantiate capability: %w", err) } diff --git a/libs/capability/runner_test.go b/libs/capability/runner_test.go index c58d5e07f..826b71f0a 100644 --- a/libs/capability/runner_test.go +++ b/libs/capability/runner_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/samber/do/v2" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -18,12 +19,9 @@ import ( // runArgs is the arguments of one run over settings a test can start: a real port, and everything // else off. -func runArgs(t *testing.T, ctor any) (*config, constructorFn, uint16) { +func runArgs(t *testing.T) (*config, uint16) { t.Helper() - c, err := newConstructor(ctor) - require.NoError(t, err) - port := uint16(freePort(t)) cfg := defaultConfig() cfg.observability.http.Port = port @@ -31,7 +29,7 @@ func runArgs(t *testing.T, ctor any) (*config, constructorFn, uint16) { // proxy, so it has to be a real listener rather than merely a lazy one. cfg.capabilities.ProxyURL = serveNodeRegistry(t, newSpyRegistry(t)) - return cfg, c, port + return cfg, port } // runToCompletion starts a run and stops it once it is serving, returning what run returned. @@ -39,7 +37,7 @@ func runArgs(t *testing.T, ctor any) (*config, constructorFn, uint16) { // A run blocks until its context is cancelled, so a test that wants one to finish has to be the // thing that ends it - standing in for the signal an operator would send. A run that fails on the // way up never gets as far as serving and reports that instead. -func runToCompletion(t *testing.T, cfg *config, c constructorFn, port uint16) error { +func runToCompletion(t *testing.T, cfg *config, c func(do.Injector) (Capability, error), port uint16) error { t.Helper() ctx, cancel := context.WithCancel(context.Background()) @@ -73,9 +71,9 @@ func runToCompletion(t *testing.T, cfg *config, c constructorFn, port uint16) er // one that gave the port back, which nothing else in the run reports. func TestRunRunsAndUnwinds(t *testing.T) { built := 0 - cfg, c, port := runArgs(t, func() *fake { built++; return newFake() }) + cfg, port := runArgs(t) - require.NoError(t, runToCompletion(t, cfg, c, port)) + require.NoError(t, runToCompletion(t, cfg, func(do.Injector) (Capability, error) { built++; return newFake(), nil }, port)) assert.Equal(t, 1, built) assertPortFree(t, port) @@ -85,9 +83,9 @@ func TestRunRunsAndUnwinds(t *testing.T) { // is running - which is what makes a health check mean anything. func TestRunStartsTheCapability(t *testing.T) { f := newFake() - cfg, c, port := runArgs(t, func() *fake { return f }) + cfg, port := runArgs(t) - require.NoError(t, runToCompletion(t, cfg, c, port)) + require.NoError(t, runToCompletion(t, cfg, func(do.Injector) (Capability, error) { return f, nil }, port)) assert.True(t, f.started, "the run should have started the capability") assert.True(t, f.closed, "and closed it again on the way out") @@ -110,16 +108,15 @@ func TestRunInstallsTelemetryBeforeBuildingTheCapability(t *testing.T) { t.Skip("builds a real beholder client; ~20s of export timeouts on close") } - cfg, _, port := runArgs(t, newFake) + cfg, port := runArgs(t) // Nothing is listening there, which is fine: the client is built and installed without dialling. cfg.observability.telemetry.Endpoint = fmt.Sprintf("localhost:%d", freePort(t)) var duringBuild *beholder.Client - c, err := newConstructor(func() *fake { + c := func(do.Injector) (Capability, error) { duringBuild = beholder.GetClient() - return newFake() - }) - require.NoError(t, err) + return newFake(), nil + } before := beholder.GetClient() require.NoError(t, runToCompletion(t, cfg, c, port)) @@ -139,7 +136,8 @@ func TestRunInstallsTelemetryBeforeBuildingTheCapability(t *testing.T) { // telemetry is built, and the undo only works once the root has started it. func TestRunUnwindsWhatStartedBeforeAFailure(t *testing.T) { before := beholder.GetClient() - cfg, c, port := runArgs(t, func() (*fake, error) { return nil, errors.New("no schedule") }) + cfg, port := runArgs(t) + c := func(do.Injector) (Capability, error) { return nil, errors.New("no schedule") } require.Empty(t, cfg.observability.telemetry.Endpoint, "these settings leave telemetry off") require.ErrorContains(t, runToCompletion(t, cfg, c, port), "no schedule") @@ -154,14 +152,14 @@ func TestRunUnwindsWhatStartedBeforeAFailure(t *testing.T) { // // It is also what proves run gets as far as the web server on the configured port at all. func TestRunReportsWhereItFailed(t *testing.T) { - cfg, c, port := runArgs(t, newFake) + cfg, port := runArgs(t) // Something else already has the port the web server is configured for. taken, err := net.Listen("tcp", fmt.Sprintf(":%d", port)) require.NoError(t, err) t.Cleanup(func() { _ = taken.Close() }) - require.ErrorContains(t, runToCompletion(t, cfg, c, port), + require.ErrorContains(t, runToCompletion(t, cfg, fakeConstructor, port), fmt.Sprintf("failed to listen on port %d", port)) } @@ -171,11 +169,11 @@ func TestRunReportsWhereItFailed(t *testing.T) { // It is also the only test that looks at a run from outside while it is up: everything is still // serving when the assertions below are made. func TestRunStaysUpUntilItIsToldToStop(t *testing.T) { - cfg, c, port := runArgs(t, newFake) + cfg, port := runArgs(t) ctx, cancel := context.WithCancel(context.Background()) done := make(chan error, 1) - go func() { done <- run(ctx, logger.Test(t), "cron", cfg, c) }() + go func() { done <- run(ctx, logger.Test(t), "cron", cfg, fakeConstructor) }() require.Eventually(t, func() bool { return serving(int(port)) }, 30*time.Second, 5*time.Millisecond, "the run should be serving while it is up") diff --git a/libs/capability/telemetry_test.go b/libs/capability/telemetry_test.go index 164cab1b1..bc38f1b5a 100644 --- a/libs/capability/telemetry_test.go +++ b/libs/capability/telemetry_test.go @@ -302,7 +302,7 @@ func TestWithOtelViewsDefaultsToNone(t *testing.T) { // config fails before a client is ever created - and an unregistered flag would have failed // earlier, with cobra's "unknown flag" instead. func TestRunUsesTheDecodedTelemetrySettings(t *testing.T) { - err := execute(t, logger.Test(t), newFake, "run", + err := execute(t, logger.Test(t), fakeConstructor, "run", "--telemetry.endpoint", "otel:4317", "--telemetry.attributes", "nope") require.ErrorContains(t, err, `invalid telemetry.attributes entry "nope": expected key=value`) @@ -385,7 +385,7 @@ func TestInstallGloballyNests(t *testing.T) { func TestRunLeavesTelemetryAsItFoundIt(t *testing.T) { before := beholder.GetClient() - require.NoError(t, execute(t, logger.Test(t), newFake, "run")) + require.NoError(t, execute(t, logger.Test(t), fakeConstructor, "run")) assert.Same(t, before, beholder.GetClient()) } diff --git a/libs/go.mod b/libs/go.mod index ca03d45fc..2f9ef087b 100644 --- a/libs/go.mod +++ b/libs/go.mod @@ -11,6 +11,7 @@ require ( github.com/hashicorp/go-plugin v1.8.0 github.com/iancoleman/strcase v0.3.0 github.com/prometheus/client_golang v1.23.2 + github.com/samber/do/v2 v2.1.0 github.com/shopspring/decimal v1.4.0 github.com/smartcontractkit/chainlink-common v0.11.2-0.20261005132539-b7d63d5fd549 github.com/smartcontractkit/chainlink-protos/cre/go v0.0.0-20260804191526-b7a850ae7648 @@ -90,6 +91,7 @@ require ( github.com/prometheus/procfs v0.20.1 // indirect github.com/sagikazarmark/locafero v0.11.0 // indirect github.com/sagikazarmark/slog-shim v0.1.0 // indirect + github.com/samber/go-type-to-string v1.8.0 // indirect github.com/santhosh-tekuri/jsonschema/v5 v5.3.1 // indirect github.com/scylladb/go-reflectx v1.0.1 // indirect github.com/smartcontractkit/chain-selectors v1.0.104 // indirect diff --git a/libs/go.sum b/libs/go.sum index 88af4233f..9843ca43e 100644 --- a/libs/go.sum +++ b/libs/go.sum @@ -220,6 +220,10 @@ github.com/sagikazarmark/locafero v0.11.0 h1:1iurJgmM9G3PA/I+wWYIOw/5SyBtxapeHDc github.com/sagikazarmark/locafero v0.11.0/go.mod h1:nVIGvgyzw595SUSUE6tvCp3YYTeHs15MvlmU87WwIik= github.com/sagikazarmark/slog-shim v0.1.0 h1:diDBnUNK9N/354PgrxMywXnAwEr1QZcOr6gto+ugjYE= github.com/sagikazarmark/slog-shim v0.1.0/go.mod h1:SrcSrq8aKtyuqEI1uvTDTK1arOWRIczQRv+GVI1AkeQ= +github.com/samber/do/v2 v2.1.0 h1:lqCHn05XvY3VqwxvZDQPSkH+jIGWSVHUrSVLEbPOopo= +github.com/samber/do/v2 v2.1.0/go.mod h1:wJBoiaZcUZyGuraOhfz15b517ZMogGs+U03DvnqvT6Q= +github.com/samber/go-type-to-string v1.8.0 h1:5z6tDTjtXxkIAoAuHAZYMYR8mkBZjVgeSH7jcSLqc8w= +github.com/samber/go-type-to-string v1.8.0/go.mod h1:jpU77vIDoIxkahknKDoEx9C8bQ1ADnh2sotZ8I4QqBU= github.com/santhosh-tekuri/jsonschema/v5 v5.3.1 h1:lZUw3E0/J3roVtGQ+SCrUrg3ON6NgVqpn3+iol9aGu4= github.com/santhosh-tekuri/jsonschema/v5 v5.3.1/go.mod h1:uToXkOrWAZ6/Oc07xWQrPOhJotwFIyu2bBVN41fcDUY= github.com/scylladb/go-reflectx v1.0.1 h1:b917wZM7189pZdlND9PbIJ6NQxfDPfBvUaQ7cjj1iZQ= From 785661837208ff359be2461add354fa7395cc987 Mon Sep 17 00:00:00 2001 From: Ryan Tinianov Date: Thu, 15 Oct 2026 15:11:35 -0400 Subject: [PATCH 2/2] Copilot feedback about nil constructor --- libs/capability/run.go | 3 +++ libs/capability/run_test.go | 11 +++++++++++ libs/capability/runner.go | 4 ++++ 3 files changed, 18 insertions(+) diff --git a/libs/capability/run.go b/libs/capability/run.go index 3e7a12bea..eaedfa75d 100644 --- a/libs/capability/run.go +++ b/libs/capability/run.go @@ -39,6 +39,9 @@ func RunErr(ctx context.Context, lggr logger.Logger, constructor func(do.Injecto if lggr == nil { return errors.New("must provide a logger") } + if constructor == nil { + return errors.New("must provide a capability constructor") + } // TODO: Run should accept some kind of Info struct root := &cobra.Command{ diff --git a/libs/capability/run_test.go b/libs/capability/run_test.go index 2bb708dea..f54125176 100644 --- a/libs/capability/run_test.go +++ b/libs/capability/run_test.go @@ -162,6 +162,10 @@ func TestRunErrRejects(t *testing.T) { t.Run("no logger", func(t *testing.T) { require.ErrorContains(t, RunErr(context.Background(), nil, fakeConstructor), "must provide a logger") }) + + t.Run("no constructor", func(t *testing.T) { + require.ErrorContains(t, RunErr(context.Background(), logger.Test(t), nil), "must provide a capability constructor") + }) } func TestRunBuildsTheCapability(t *testing.T) { @@ -181,6 +185,13 @@ func TestRunReportsAFailedConstructor(t *testing.T) { require.ErrorContains(t, err, "failed to instantiate capability: no schedule") } +// TestRunRejectsANilCapability covers a constructor that reports success without building +// anything: the run says so rather than starting a nil capability. +func TestRunRejectsANilCapability(t *testing.T) { + err := execute(t, logger.Test(t), func(do.Injector) (Capability, error) { return nil, nil }, "run") + require.ErrorContains(t, err, "returned neither a capability nor an error") +} + // TestRunProvidesItsDependencies covers what the injector holds: this process's logger, the // registry capabilities are resolved through, and the factory a workflow's requests are bounded by. func TestRunProvidesItsDependencies(t *testing.T) { diff --git a/libs/capability/runner.go b/libs/capability/runner.go index caf212c0d..d0a7dd51a 100644 --- a/libs/capability/runner.go +++ b/libs/capability/runner.go @@ -2,6 +2,7 @@ package capability import ( "context" + "errors" "fmt" "net/http" "os" @@ -119,6 +120,9 @@ func startCapability(ctx context.Context, lggr logger.Logger, cfg *config, ctor if err != nil { return nil, fmt.Errorf("failed to instantiate capability: %w", err) } + if c == nil { + return nil, errors.New("failed to instantiate capability: the constructor returned neither a capability nor an error") + } if err := c.Start(ctx); err != nil { return nil, fmt.Errorf("failed to start capability: %w", err)