diff --git a/orchestrator/plugin.go b/orchestrator/plugin.go index ec824ba..c40315f 100644 --- a/orchestrator/plugin.go +++ b/orchestrator/plugin.go @@ -9,9 +9,9 @@ import ( "strings" "github.com/GoCodeAlone/modular" - "github.com/GoCodeAlone/workflow-plugin-agent/provider" - "github.com/GoCodeAlone/workflow-plugin-agent/orchestrator/tools" agentplugin "github.com/GoCodeAlone/workflow-plugin-agent" + "github.com/GoCodeAlone/workflow-plugin-agent/orchestrator/tools" + "github.com/GoCodeAlone/workflow-plugin-agent/provider" "github.com/GoCodeAlone/workflow-plugin-authz/authz" "github.com/GoCodeAlone/workflow/capability" "github.com/GoCodeAlone/workflow/config" @@ -97,12 +97,12 @@ func (p *RatchetPlugin) StepFactories() map[string]plugin.StepFactory { "step.bcrypt_hash": newBcryptHashFactory(), "step.jwt_generate": newJWTGenerateFactory(), "step.jwt_decode": newJWTDecodeFactory(), - "step.blackboard_post": newBlackboardPostFactory(), - "step.blackboard_read": newBlackboardReadFactory(), - "step.self_improve_validate": newSelfImproveValidateFactory(), - "step.self_improve_diff": newSelfImproveDiffFactory(), - "step.self_improve_deploy": newSelfImproveDeployFactory(), - "step.lsp_diagnose": newLSPDiagnoseFactory(), + "step.blackboard_post": newBlackboardPostFactory(), + "step.blackboard_read": newBlackboardReadFactory(), + "step.self_improve_validate": newSelfImproveValidateFactory(), + "step.self_improve_diff": newSelfImproveDiffFactory(), + "step.self_improve_deploy": newSelfImproveDeployFactory(), + "step.lsp_diagnose": newLSPDiagnoseFactory(), } // Merge in authz step factories (step.authz_check_casbin, step.authz_add_policy, etc.) @@ -188,68 +188,46 @@ func (p *RatchetPlugin) ModuleSchemas() []*schema.ModuleSchema { } // secretsGuardHook creates a SecretGuard and registers it in the service registry. -// It defaults to vault-dev (managed HashiCorp Vault dev server). -// Backend selection priority: -// 1. data/vault-config.json (vault-remote or vault-dev) -// 2. Default: vault-dev -// 3. Fallback: FileProvider if vault binary is not available // -// Also loads RATCHET_* environment variables for backward compatibility. +// Phase 3 (D19): the vault backend is now owned by the engine secrets.vault +// module (declared by the host in config), NOT constructed inline here. Because +// wiring hooks run in BuildFromConfig BEFORE module Start() — so the engine +// module's Provider() is nil at hook time — the guard is armed to LAZILY resolve +// the module's Provider on its first redaction call (post-Start) via +// AttachLazyVault. The vault module service key defaults to "vault" and can be +// overridden via the RATCHET_VAULT_MODULE env var. +// +// P13 (preserved): RATCHET_* env-var secrets + a remote-vault token (when a +// saved vault-config is present) are still loaded into the guard's knownValues +// for free-text redaction — this is orthogonal to the Provider source and must +// not regress. func secretsGuardHook() plugin.WiringHook { return plugin.WiringHook{ Name: "ratchet.secrets_guard", Priority: 85, Hook: func(app modular.Application, _ *config.WorkflowConfig) error { - var sp secrets.Provider - backendName := "vault-dev" - - // Check for saved vault config - vcfg, _ := LoadVaultConfig(vaultConfigDir()) - - if vcfg != nil && vcfg.Backend == "vault-remote" && vcfg.Address != "" && vcfg.Token != "" { - // Use remote vault from saved config - vp, err := secrets.NewVaultProvider(secrets.VaultConfig{ - Address: vcfg.Address, - Token: vcfg.Token, - MountPath: vcfg.MountPath, - Namespace: vcfg.Namespace, - }) - if err != nil { - app.Logger().Warn("vault-remote config found but connection failed, falling back to vault-dev", "error", err) - } else { - sp = vp - backendName = "vault-remote" - } - } + // No inline provider construction: the engine secrets.vault module + // owns the backend. The guard resolves it lazily on first redaction. + guard := NewSecretGuard(nil, "") - // Default to vault-dev if no remote configured - if sp == nil { - dp, err := secrets.NewDevVaultProvider(secrets.DevVaultConfig{}) - if err != nil { - app.Logger().Warn("vault-dev not available (vault binary not found), falling back to file provider", "error", err) - sp = newFileProvider(app) - backendName = "file" - } else { - sp = dp - backendName = "vault-dev" - _ = app.RegisterService("ratchet-vault-dev", dp) - } + // Arm lazy resolution against the engine secrets.vault module. + vaultModuleKey := os.Getenv("RATCHET_VAULT_MODULE") + if vaultModuleKey == "" { + vaultModuleKey = "vault" } - - guard := NewSecretGuard(sp, backendName) + guard.AttachLazyVault(app, vaultModuleKey) ctx := context.Background() - // Load all secrets from the provider - _ = guard.LoadAllSecrets(ctx) - - // Register vault token for redaction if using remote vault - if vcfg != nil && vcfg.Token != "" { + // P13: register a remote-vault token for redaction if a saved config + // is present (the token itself must never leak into LLM output even + // though the engine module now owns the connection). + if vcfg, _ := LoadVaultConfig(vaultConfigDir()); vcfg != nil && vcfg.Token != "" { guard.AddKnownSecret("VAULT_TOKEN", vcfg.Token) } - // Backward compat: also load RATCHET_* env vars into SecretGuard - // (These are loaded for redaction only; the env provider is not the primary store.) + // P13: load RATCHET_* env vars into the guard for free-text redaction. + // (Loaded for redaction only; the env provider is not the primary store.) envProvider := secrets.NewEnvProvider("RATCHET_") for _, env := range os.Environ() { if strings.HasPrefix(env, "RATCHET_") { @@ -261,27 +239,15 @@ func secretsGuardHook() plugin.WiringHook { } } - app.Logger().Info("secrets backend initialized", "backend", backendName) + app.Logger().Info("secrets guard armed (lazy-resolve engine vault module)", "vault_module", vaultModuleKey) - // Register in service registry + // Register in service registry (the (R) steps resolve this). _ = app.RegisterService("ratchet-secret-guard", guard) return nil }, } } -// newFileProvider creates the default FileProvider for secrets storage. -func newFileProvider(app modular.Application) secrets.Provider { - secretsDir := os.Getenv("RATCHET_SECRETS_DIR") - if secretsDir == "" { - secretsDir = "data/secrets" - } - if err := os.MkdirAll(secretsDir, 0700); err != nil { - app.Logger().Warn("failed to create secrets dir", "error", err) - } - return secrets.NewFileProvider(secretsDir) -} - // providerRegistryHook creates a ProviderRegistry and registers it in the service registry. func providerRegistryHook() plugin.WiringHook { return plugin.WiringHook{ @@ -299,15 +265,22 @@ func providerRegistryHook() plugin.WiringHook { return nil // no DB, skip } - // Get secrets provider from SecretGuard - var sp secrets.Provider + // Get secrets provider ACCESSOR from SecretGuard. + // + // We pass the guard.Provider METHOD VALUE (not the call result) because + // wiring hooks run in BuildFromConfig BEFORE module Start() — so the + // engine secrets.vault module's Provider() is nil at hook time. The + // guard lazy-resolves it on first use (post-Start); resolving on demand + // at provider-resolution time (e.g. agent_execute) triggers that + // lazy-resolve instead of snapshotting a permanently-nil provider. + var spAccessor func() secrets.Provider if svc, ok := app.SvcRegistry()["ratchet-secret-guard"]; ok { if guard, ok := svc.(*SecretGuard); ok { - sp = guard.Provider() + spAccessor = guard.Provider } } - registry := NewProviderRegistry(db, sp) + registry := NewProviderRegistry(db, spAccessor) _ = app.RegisterService("ratchet-provider-registry", registry) return nil }, diff --git a/orchestrator/provider_registry.go b/orchestrator/provider_registry.go index 40b9898..081568d 100644 --- a/orchestrator/provider_registry.go +++ b/orchestrator/provider_registry.go @@ -42,22 +42,40 @@ func (c *LLMProviderConfig) settings() map[string]string { type ProviderFactory func(ctx context.Context, apiKey string, cfg LLMProviderConfig) (provider.Provider, error) // ProviderRegistry manages AI provider lifecycle: factory creation, caching, and DB lookup. +// +// The secrets provider is held as an ACCESSOR (func() secrets.Provider) rather +// than a snapshot because wiring hooks run in BuildFromConfig BEFORE module +// Start() — so the engine secrets.vault module's Provider() is nil at hook time +// (the SecretGuard lazy-resolves it on first use, post-Start). Resolving on +// demand at provider-resolution time (post-Start, e.g. inside agent_execute) +// triggers that lazy-resolve instead of snapshotting a permanently-nil provider. +// UpdateSecretsProvider hot-saps the accessor for the runtime vault-config path. type ProviderRegistry struct { - mu sync.RWMutex - db *sql.DB - secrets secrets.Provider - cache map[string]provider.Provider - factories map[string]ProviderFactory - sflight singleflight.Group // deduplicates concurrent cold-start creation per alias + mu sync.RWMutex + db *sql.DB + secretsProvider func() secrets.Provider + cache map[string]provider.Provider + factories map[string]ProviderFactory + sflight singleflight.Group // deduplicates concurrent cold-start creation per alias } // NewProviderRegistry creates a new ProviderRegistry with built-in factories registered. -func NewProviderRegistry(db *sql.DB, secretsProvider secrets.Provider) *ProviderRegistry { +// +// secretsProvider is a lazy accessor (typically guard.Provider) — it is invoked +// on demand at provider-resolution time (post-Start), NOT snapshotted at wiring +// time. This is required because the SecretGuard resolves the engine vault +// module's Provider lazily on first use; snapshotting at wiring time would +// capture nil (the module's Provider is populated only in Start(), which runs +// AFTER wiring hooks). A nil accessor degrades gracefully (no secret resolution). +func NewProviderRegistry(db *sql.DB, secretsProvider func() secrets.Provider) *ProviderRegistry { + if secretsProvider == nil { + secretsProvider = func() secrets.Provider { return nil } + } r := &ProviderRegistry{ - db: db, - secrets: secretsProvider, - cache: make(map[string]provider.Provider), - factories: make(map[string]ProviderFactory), + db: db, + secretsProvider: secretsProvider, + cache: make(map[string]provider.Provider), + factories: make(map[string]ProviderFactory), } // Register built-in factories @@ -128,9 +146,11 @@ func (r *ProviderRegistry) GetDefault(ctx context.Context) (provider.Provider, e } // UpdateSecretsProvider swaps the underlying secrets provider and clears the cache. +// It overrides the lazy accessor with one returning the hot-swapped provider, +// so the runtime vault-config path takes precedence over guard lazy-resolution. func (r *ProviderRegistry) UpdateSecretsProvider(p secrets.Provider) { r.mu.Lock() - r.secrets = p + r.secretsProvider = func() secrets.Provider { return p } r.cache = make(map[string]provider.Provider) r.mu.Unlock() } @@ -227,13 +247,20 @@ func (r *ProviderRegistry) createAndCache(ctx context.Context, alias string, cfg } r.mu.RUnlock() - // Resolve API key from secrets + // Resolve API key from secrets — invoke the lazy accessor (post-Start) + // so the SecretGuard's lazy-resolve fires at provider-resolution time, + // not the wiring-time nil snapshot. var apiKey string - if cfg.SecretName != "" && r.secrets != nil { - var err error - apiKey, err = r.secrets.Get(ctx, cfg.SecretName) - if err != nil { - return nil, fmt.Errorf("provider registry: resolve secret %q: %w", cfg.SecretName, err) + r.mu.RLock() + spAccessor := r.secretsProvider + r.mu.RUnlock() + if cfg.SecretName != "" { + if sp := spAccessor(); sp != nil { + var err error + apiKey, err = sp.Get(ctx, cfg.SecretName) + if err != nil { + return nil, fmt.Errorf("provider registry: resolve secret %q: %w", cfg.SecretName, err) + } } } diff --git a/orchestrator/provider_registry_test.go b/orchestrator/provider_registry_test.go index a4c410d..a5ebb5f 100644 --- a/orchestrator/provider_registry_test.go +++ b/orchestrator/provider_registry_test.go @@ -6,10 +6,18 @@ import ( "testing" "github.com/GoCodeAlone/workflow-plugin-agent/provider" + "github.com/GoCodeAlone/workflow/secrets" _ "modernc.org/sqlite" ) +// newProviderRegistry wraps a concrete secrets.Provider in an accessor for +// tests that don't exercise the lazy path (back-compat with the pre-fix call +// shape NewProviderRegistry(db, sec)). +func newProviderRegistry(db *sql.DB, sec secrets.Provider) *ProviderRegistry { + return NewProviderRegistry(db, func() secrets.Provider { return sec }) +} + // memSecretsProvider is an in-memory secrets provider for testing. type memSecretsProvider struct { data map[string]string @@ -81,7 +89,7 @@ func TestProviderRegistryGetByAlias_Mock(t *testing.T) { t.Fatalf("insert: %v", err) } - reg := NewProviderRegistry(db, secrets) + reg := newProviderRegistry(db, secrets) p, err := reg.GetByAlias(context.Background(), "test-mock") if err != nil { t.Fatalf("GetByAlias: %v", err) @@ -112,7 +120,7 @@ func TestProviderRegistryGetByAlias_Anthropic(t *testing.T) { t.Fatalf("insert: %v", err) } - reg := NewProviderRegistry(db, sec) + reg := newProviderRegistry(db, sec) p, err := reg.GetByAlias(context.Background(), "my-claude") if err != nil { t.Fatalf("GetByAlias: %v", err) @@ -131,7 +139,7 @@ func TestProviderRegistryGetDefault(t *testing.T) { t.Fatalf("insert: %v", err) } - reg := NewProviderRegistry(db, sec) + reg := newProviderRegistry(db, sec) p, err := reg.GetDefault(context.Background()) if err != nil { t.Fatalf("GetDefault: %v", err) @@ -145,7 +153,7 @@ func TestProviderRegistryGetDefault_NoDefault(t *testing.T) { db := setupTestDB(t) sec := &memSecretsProvider{data: map[string]string{}} - reg := NewProviderRegistry(db, sec) + reg := newProviderRegistry(db, sec) _, err := reg.GetDefault(context.Background()) if err == nil { t.Fatal("expected error when no default provider exists") @@ -156,7 +164,7 @@ func TestProviderRegistryNotFound(t *testing.T) { db := setupTestDB(t) sec := &memSecretsProvider{data: map[string]string{}} - reg := NewProviderRegistry(db, sec) + reg := newProviderRegistry(db, sec) _, err := reg.GetByAlias(context.Background(), "nonexistent") if err == nil { t.Fatal("expected error for nonexistent alias") @@ -172,7 +180,7 @@ func TestProviderRegistryInvalidateCache(t *testing.T) { t.Fatalf("insert: %v", err) } - reg := NewProviderRegistry(db, sec) + reg := newProviderRegistry(db, sec) // Populate cache p1, err := reg.GetByAlias(context.Background(), "cached") @@ -205,7 +213,7 @@ func TestProviderRegistryInvalidateCacheBySecret(t *testing.T) { t.Fatalf("insert: %v", err) } - reg := NewProviderRegistry(db, sec) + reg := newProviderRegistry(db, sec) // Populate cache _, err = reg.GetByAlias(context.Background(), "uses-secret") @@ -241,7 +249,7 @@ func TestProviderRegistryUnknownType(t *testing.T) { t.Fatalf("insert: %v", err) } - reg := NewProviderRegistry(db, sec) + reg := newProviderRegistry(db, sec) _, err = reg.GetByAlias(context.Background(), "bad-type") if err == nil { t.Fatal("expected error for unknown provider type") @@ -257,7 +265,7 @@ func TestProviderRegistryTestConnection(t *testing.T) { t.Fatalf("insert: %v", err) } - reg := NewProviderRegistry(db, sec) + reg := newProviderRegistry(db, sec) ok, msg, latency, err := reg.TestConnection(context.Background(), "test-conn") if err != nil { t.Fatalf("TestConnection: %v", err) @@ -301,7 +309,7 @@ func TestProviderRegistryNewProviderTypes(t *testing.T) { t.Fatalf("insert: %v", err) } - reg := NewProviderRegistry(db, sec) + reg := newProviderRegistry(db, sec) p, err := reg.GetByAlias(t.Context(), tt.name) if err != nil { t.Fatalf("GetByAlias: %v", err) @@ -316,7 +324,7 @@ func TestProviderRegistryNewProviderTypes(t *testing.T) { func TestProviderRegistryVertexFactoryRegistered(t *testing.T) { db := setupTestDB(t) sec := &memSecretsProvider{data: map[string]string{}} - reg := NewProviderRegistry(db, sec) + reg := newProviderRegistry(db, sec) if _, ok := reg.factories["anthropic_vertex"]; !ok { t.Fatal("anthropic_vertex factory not registered") @@ -346,7 +354,7 @@ func TestProviderRegistryChatMock(t *testing.T) { t.Fatalf("insert: %v", err) } - reg := NewProviderRegistry(db, sec) + reg := newProviderRegistry(db, sec) p, err := reg.GetByAlias(context.Background(), "chat-mock") if err != nil { t.Fatalf("GetByAlias: %v", err) diff --git a/orchestrator/ratchetplugin_test.go b/orchestrator/ratchetplugin_test.go index c720506..79ff441 100644 --- a/orchestrator/ratchetplugin_test.go +++ b/orchestrator/ratchetplugin_test.go @@ -275,6 +275,91 @@ func TestSecretGuard_ConcurrentAccess(t *testing.T) { wg.Wait() } +// --------------------------------------------------------------------------- +// secretsGuardHook slimming tests (Phase 3, Task 11 / D19 / P13) +// --------------------------------------------------------------------------- + +// TestSecretsGuardHook_NoVaultDevRegistration proves the slimmed hook no longer +// spins up vault-dev or registers a "ratchet-vault-dev" service. The engine +// secrets.vault module (declared by the host in config) owns the backend; the +// guard lazy-resolves it. +func TestSecretsGuardHook_NoVaultDevRegistration(t *testing.T) { + // Isolate from any host vault config / RATCHET_ env so the test is deterministic. + t.Setenv("RATCHET_SECRETS_DIR", t.TempDir()) + app := newMockApp() + + hook := secretsGuardHook() + if err := hook.Hook(app, nil); err != nil { + t.Fatalf("secretsGuardHook: %v", err) + } + + if _, ok := app.services["ratchet-vault-dev"]; ok { + t.Error("slimmed hook must NOT register ratchet-vault-dev; engine secrets.vault module owns the backend") + } +} + +// TestSecretsGuardHook_PreservesEnvVarRedaction proves the slimmed hook STILL +// populates the guard's knownValues from RATCHET_* env vars (P13), so secrets +// held in env vars remain redacted even though the vault provider is now lazily +// resolved from the engine module. +func TestSecretsGuardHook_PreservesEnvVarRedaction(t *testing.T) { + const envVal = "env-held-super-secret-token" + t.Setenv("RATCHET_API_KEY", envVal) + t.Setenv("RATCHET_SECRETS_DIR", t.TempDir()) + + app := newMockApp() + if err := secretsGuardHook().Hook(app, nil); err != nil { + t.Fatalf("secretsGuardHook: %v", err) + } + + guardSvc, ok := app.services["ratchet-secret-guard"] + if !ok { + t.Fatal("ratchet-secret-guard service not registered") + } + guard, ok := guardSvc.(*SecretGuard) + if !ok { + t.Fatalf("ratchet-secret-guard is %T, want *SecretGuard", guardSvc) + } + + // The env-held value must be redacted without any lazy-resolve having fired + // (no vault module is registered, so resolve() no-ops). + redacted := guard.Redact("leaked: " + envVal) + if strings.Contains(redacted, envVal) { + t.Errorf("env-held secret value leaked: %q", redacted) + } + if !strings.Contains(redacted, "[REDACTED:") { + t.Errorf("expected env value to be redacted, got %q", redacted) + } +} + +// TestSecretsGuardHook_ArmsLazyVaultResolve proves the slimmed hook arms the +// guard for lazy resolution against the engine secrets.vault module, so a +// module registered post-hook (post-Start) is picked up on first redaction. +func TestSecretsGuardHook_ArmsLazyVaultResolve(t *testing.T) { + t.Setenv("RATCHET_SECRETS_DIR", t.TempDir()) + app := newMockApp() + + if err := secretsGuardHook().Hook(app, nil); err != nil { + t.Fatalf("secretsGuardHook: %v", err) + } + + guard := app.services["ratchet-secret-guard"].(*SecretGuard) + + // Simulate the engine module starting AFTER the hook ran (post-Start) by + // registering a provider-shaped module under the default vault key now. + const vaultKey = "vault" + known := &mockSecretsProvider{secrets: map[string]string{"ENGINE_SECRET": "engine-secret-value"}} + app.services[vaultKey] = &fakeVaultModule{name: vaultKey, p: known} + + redacted := guard.Redact("contains engine-secret-value") + if strings.Contains(redacted, "engine-secret-value") { + t.Errorf("engine-module secret value leaked after lazy resolve: %q", redacted) + } + if !strings.Contains(redacted, "[REDACTED:ENGINE_SECRET]") { + t.Errorf("expected lazy-resolved engine secret to be redacted, got %q", redacted) + } +} + // --------------------------------------------------------------------------- // ToolRegistry tests // --------------------------------------------------------------------------- diff --git a/orchestrator/secret_guard.go b/orchestrator/secret_guard.go index 8dff28b..9d7188e 100644 --- a/orchestrator/secret_guard.go +++ b/orchestrator/secret_guard.go @@ -5,16 +5,34 @@ import ( "strings" "sync" + "github.com/GoCodeAlone/modular" "github.com/GoCodeAlone/workflow-plugin-agent/provider" "github.com/GoCodeAlone/workflow/secrets" ) +// vaultProvider exposes the Provider() accessor the lazy-resolver type-asserts +// on. The engine module.SecretsVaultModule implements this shape; defining it as +// a local interface keeps the dependency one-directional (we assert against a +// structural interface rather than importing the concrete module type, which +// would create an import cycle since workflow/module is a heavy dep). +type vaultProvider interface { + Provider() secrets.Provider +} + // SecretGuard scans text for known secret values and redacts them. type SecretGuard struct { mu sync.RWMutex knownValues map[string]string // value → name (reversed for fast lookup) provider secrets.Provider backendName string + + // Lazy-resolve fields (D19). Wiring hooks run in BuildFromConfig BEFORE + // module Start(), so the engine secrets.vault module's Provider() is nil at + // hook time. The guard therefore lazily resolves it on the first redaction + // call (which executes post-Start) via sync.Once. + app modular.Application + vaultModuleKey string + resolveOnce sync.Once } func NewSecretGuard(p secrets.Provider, backend string) *SecretGuard { @@ -25,6 +43,51 @@ func NewSecretGuard(p secrets.Provider, backend string) *SecretGuard { } } +// AttachLazyVault arms the guard to lazily resolve the engine secrets.vault +// module's Provider from the service registry on the first redaction call. +// This is the D19 fix for the lifecycle inversion: wiring hooks run pre-Start +// (the module's Provider() is nil at hook time), so resolution is deferred to +// the first post-Start redaction. vaultModuleKey is the module's config name +// (the host declares it; default "vault"). Safe to call with a nil app or empty +// key — resolve() becomes a no-op and redaction falls back to AddKnownSecret +// values (the env-var redaction path). +func (sg *SecretGuard) AttachLazyVault(app modular.Application, vaultModuleKey string) { + sg.mu.Lock() + defer sg.mu.Unlock() + sg.app = app + sg.vaultModuleKey = vaultModuleKey +} + +// resolve looks up the engine secrets.vault module in the service registry and, +// if present and provider-shaped, hot-swaps it in via SetProvider (which +// pre-loads knownValues + atomic-swaps). It is invoked exactly once by +// resolveOnce.Do from Redact/CheckAndRedact. Failures (nil app, missing key, +// wrong shape) are silent no-ops — env-var-added values remain redacted. +func (sg *SecretGuard) resolve() { + // Read app + key without holding the lock during the registry lookup + + // SetProvider (SetProvider takes its own write lock). sync.Once serializes + // concurrent first-callers so there is no read-during-populate race. + sg.mu.RLock() + app := sg.app + key := sg.vaultModuleKey + sg.mu.RUnlock() + + if app == nil || key == "" { + return + } + svc, ok := app.SvcRegistry()[key] + if !ok { + return + } + vp, ok := svc.(vaultProvider) + if !ok { + return + } + if p := vp.Provider(); p != nil { + sg.SetProvider(p, "vault") + } +} + // SetProvider hot-swaps the secrets provider and reloads all secrets. // Secrets are loaded from the new provider before swapping to avoid // a window where no secrets are available for redaction. @@ -87,6 +150,12 @@ func (sg *SecretGuard) LoadAllSecrets(ctx context.Context) error { // Redact replaces known secret values with [REDACTED:name]. func (sg *SecretGuard) Redact(text string) string { + // Lazy-resolve the engine secrets.vault module's Provider on the first call + // (post-Start). sync.Once.Do serializes concurrent first-callers; SetProvider + // takes its own write lock (released before Once.Do returns), so subsequent + // callers safely take the RLock below. + sg.resolveOnce.Do(sg.resolve) + sg.mu.RLock() defer sg.mu.RUnlock() for val, name := range sg.knownValues { diff --git a/orchestrator/secret_guard_ext.go b/orchestrator/secret_guard_ext.go index 18a0692..5430397 100644 --- a/orchestrator/secret_guard_ext.go +++ b/orchestrator/secret_guard_ext.go @@ -3,7 +3,15 @@ package orchestrator import "github.com/GoCodeAlone/workflow/secrets" // Provider returns the underlying secrets.Provider. +// +// It triggers the lazy-resolve of the engine secrets.vault module's Provider on +// first call (post-Start) — the same pathway as Redact. This matters because +// ProviderRegistry resolves secrets on demand via this accessor (post-Start, +// e.g. inside agent_execute); the lazy-resolve must fire here too, not only on +// redaction, otherwise vault-backed AI providers would silently get an empty +// API key (the regression fixed alongside the PR4 lazy-resolve change). func (sg *SecretGuard) Provider() secrets.Provider { + sg.resolveOnce.Do(sg.resolve) sg.mu.RLock() defer sg.mu.RUnlock() return sg.provider diff --git a/orchestrator/secret_guard_lazy_test.go b/orchestrator/secret_guard_lazy_test.go new file mode 100644 index 0000000..3693a28 --- /dev/null +++ b/orchestrator/secret_guard_lazy_test.go @@ -0,0 +1,322 @@ +package orchestrator + +import ( + "context" + "strings" + "sync" + "testing" + + "github.com/GoCodeAlone/modular" + "github.com/GoCodeAlone/workflow-plugin-agent/provider" + "github.com/GoCodeAlone/workflow/secrets" +) + +// fakeVaultModule mimics the engine module.SecretsVaultModule shape: it is +// registered under its config name in the service registry and exposes a +// Provider() secrets.Provider accessor that the lazy-resolver type-asserts on. +type fakeVaultModule struct { + name string + p secrets.Provider +} + +func (f *fakeVaultModule) Provider() secrets.Provider { return f.p } + +// lazyStubApp is a minimal modular.Application whose SvcRegistry returns the +// vault-module-shaped service the lazy-resolver looks up. It reuses the +// embedded modular.Application pattern from mockApp so it satisfies the full +// interface. +type lazyStubApp struct { + modular.Application + services map[string]any +} + +func (a *lazyStubApp) SvcRegistry() modular.ServiceRegistry { + return modular.ServiceRegistry(a.services) +} + +func (a *lazyStubApp) RegisterService(name string, svc any) error { + a.services[name] = svc + return nil +} + +func (a *lazyStubApp) Logger() modular.Logger { return &noopLogger{} } + +// TestSecretGuardLazyResolve_PopulatesFromEngineModule proves the guard +// lazily resolves the engine secrets.vault module's Provider on first redaction +// call (post-Start lifecycle) and pre-loads knownValues from it. +func TestSecretGuardLazyResolve_PopulatesFromEngineModule(t *testing.T) { + const vaultKey = "vault" + known := &mockSecretsProvider{secrets: map[string]string{ + "DATABASE_URL": "postgres://super-secret-db-host:5432/prod", + }} + // Pre-Start: module registered but Provider() is nil; Start() populates it. + vaultMod := &startableVaultModule{} + + app := &lazyStubApp{services: map[string]any{vaultKey: vaultMod}} + + // Construct the guard WITHOUT a provider (pre-Start hook wiring) and arm it + // for lazy resolution against the engine module. + sg := NewSecretGuard(nil, "") + sg.AttachLazyVault(app, vaultKey) + + // module.Start() populates the engine module's Provider post-wiring. + vaultMod.Start(known) + + // First redaction call must trigger lazy resolve -> SetProvider -> knownValues populated. + msg := &provider.Message{Content: "the connection string is postgres://super-secret-db-host:5432/prod please"} + changed := sg.CheckAndRedact(msg) + if !changed { + t.Fatal("expected CheckAndRedact to redact after lazy resolve, got no change") + } + if strings.Contains(msg.Content, "super-secret-db-host") { + t.Errorf("secret value still present after lazy redact: %q", msg.Content) + } + if !strings.Contains(msg.Content, "[REDACTED:DATABASE_URL]") { + t.Errorf("expected [REDACTED:DATABASE_URL], got %q", msg.Content) + } + + // Provider must now be the engine module's provider, backend labeled "vault". + if sg.Provider() != known { + t.Errorf("provider not resolved to engine module's provider after lazy resolve") + } + if got := sg.BackendName(); got != "vault" { + t.Errorf("backend name: got %q, want %q", got, "vault") + } +} + +// TestSecretGuardLazyResolve_NilAppNoPanic proves resolve() is a safe no-op when +// no app/module is attached (env-var-only redaction path), and redaction is a +// pass-through rather than a panic. +func TestSecretGuardLazyResolve_NilAppNoPanic(t *testing.T) { + sg := NewSecretGuard(nil, "") + // No AttachLazyVault call -> app == nil, vaultModuleKey == "". + // Manually pre-seed one known value (env-var path) to confirm Redact still + // works against values added via AddKnownSecret even when lazy resolve no-ops. + sg.AddKnownSecret("ENV_TOKEN", "env-secret-value") + + text := "contains env-secret-value and an unknown value" + got := sg.Redact(text) + if !strings.Contains(got, "[REDACTED:ENV_TOKEN]") { + t.Errorf("AddKnownSecret value not redacted on nil-app path: %q", got) + } + if strings.Contains(got, "env-secret-value") { + t.Errorf("env secret value leaked: %q", got) + } +} + +// TestSecretGuardLazyResolve_NilAppEmptyKnownValuesPassThrough confirms that +// when app is nil AND knownValues is empty, Redact is a clean pass-through +// (no panic, no mutation). +func TestSecretGuardLazyResolve_NilAppEmptyKnownValuesPassThrough(t *testing.T) { + sg := NewSecretGuard(nil, "") + text := "nothing to redact" + got := sg.Redact(text) + if got != text { + t.Errorf("expected unchanged pass-through, got %q", got) + } +} + +// TestSecretGuardLazyResolve_OnceGuarantee confirms lazy resolve runs exactly +// once even under concurrent first-call contention (sync.Once semantics), and +// the resolver does not deadlock under -race. +func TestSecretGuardLazyResolve_OnceGuarantee(t *testing.T) { + const vaultKey = "vault" + known := &mockSecretsProvider{secrets: map[string]string{"K": "val-k"}} + vaultMod := &fakeVaultModule{name: vaultKey, p: known} + app := &lazyStubApp{services: map[string]any{vaultKey: vaultMod}} + + sg := NewSecretGuard(nil, "") + sg.AttachLazyVault(app, vaultKey) + + var wg sync.WaitGroup + for i := 0; i < 20; i++ { + wg.Add(1) + go func() { + defer wg.Done() + got := sg.Redact("contains val-k") + if !strings.Contains(got, "[REDACTED:K]") { + t.Errorf("concurrent redact missed value: %q", got) + } + }() + } + wg.Wait() +} + +// TestSecretGuardLazyResolve_ModuleMissingNoPanic proves that if the configured +// vault module key is absent from the registry (host didn't declare secrets.vault), +// resolve() is a safe no-op and redaction falls back to AddKnownSecret values. +func TestSecretGuardLazyResolve_ModuleMissingNoPanic(t *testing.T) { + app := &lazyStubApp{services: map[string]any{}} // no vault key + sg := NewSecretGuard(nil, "") + sg.AttachLazyVault(app, "vault") + sg.AddKnownSecret("MANUAL", "manual-val") + + got := sg.Redact("manual-val present") + if !strings.Contains(got, "[REDACTED:MANUAL]") { + t.Errorf("manual value not redacted when module missing: %q", got) + } +} + +// TestSecretGuardLazyResolve_ModuleNotProviderShape proves a service registered +// under the vault key that does NOT expose Provider() is safely skipped (no panic). +func TestSecretGuardLazyResolve_ModuleNotProviderShape(t *testing.T) { + app := &lazyStubApp{services: map[string]any{"vault": "not-a-vault-module"}} + sg := NewSecretGuard(nil, "") + sg.AttachLazyVault(app, "vault") + + // Must not panic; redaction is a no-op pass-through. + text := "leaked-value" + if got := sg.Redact(text); got != text { + t.Errorf("expected pass-through for non-provider module shape, got %q", got) + } +} + +// startableVaultModule models the engine secrets.vault module lifecycle: it is +// registered in the service registry at init time but its Provider() is nil +// until Start() is called (post-wiring). This lets the registry test faithfully +// reproduce the pre-Start (nil) → post-Start (populated) transition. +type startableVaultModule struct { + p secrets.Provider +} + +func (m *startableVaultModule) Provider() secrets.Provider { return m.p } + +// Start mirrors module.Start(): populates the Provider (post-wiring). +func (m *startableVaultModule) Start(p secrets.Provider) { m.p = p } + +// TestProviderRegistryResolvesSecretViaLazyGuard proves the ProviderRegistry +// resolves a vault-backed secret via the LAZY SecretGuard accessor (the method +// value guard.Provider), NOT a nil wiring-time snapshot. +// +// This is the regression test for the PR4 lazy-resolve change: providerRegistryHook +// runs at wiring time (pre-Start), where the engine secrets.vault module's +// Provider() is nil. The registry must defer resolution to provider-resolution +// time (post-Start) so vault-backed AI providers (those with SecretName) get +// their real API key instead of an empty one. +func TestProviderRegistryResolvesSecretViaLazyGuard(t *testing.T) { + const vaultKey = "vault" + known := &mockSecretsProvider{secrets: map[string]string{ + "ANTHROPIC_API_KEY": "sk-vault-resolved-key", + }} + // Pre-Start: module registered, Provider() nil. + vaultMod := &startableVaultModule{} + app := &lazyStubApp{services: map[string]any{vaultKey: vaultMod}} + + // === WIRING TIME (pre-Start) === + // Construct the guard exactly as secretsGuardHook does: no pre-set provider, + // armed for lazy resolution against the engine module. + guard := NewSecretGuard(nil, "") + guard.AttachLazyVault(app, vaultKey) + + // The registry is built with the guard.Provider METHOD VALUE (lazy accessor), + // exactly as providerRegistryHook now wires it. It must NOT snapshot nil here. + reg := NewProviderRegistry(setupTestDB(t), guard.Provider) + + _, err := reg.db.Exec(`INSERT INTO llm_providers (id, alias, type, model, secret_name) + VALUES ('p1', 'vaulted-claude', 'anthropic', 'claude-sonnet-4-20250514', 'ANTHROPIC_API_KEY')`) + if err != nil { + t.Fatalf("insert: %v", err) + } + + // === POST-START === + // module.Start() runs after wiring hooks and populates the module's Provider. + vaultMod.Start(known) + + // === PROVIDER-RESOLUTION TIME (post-Start, e.g. agent_execute) === + // Resolve the provider. The registry invokes the lazy accessor, which triggers + // the guard's lazy-resolve of the now-populated engine module Provider. If the + // registry had snapshotted nil at wiring time, the API key would be empty and + // the anthropic factory would reject it ("APIKey is required"). + p, err := reg.GetByAlias(context.Background(), "vaulted-claude") + if err != nil { + t.Fatalf("GetByAlias via lazy guard: %v", err) + } + if p == nil { + t.Fatal("expected non-nil provider resolved via lazy guard") + } + if p.Name() != "anthropic" { + t.Errorf("expected anthropic provider, got %q", p.Name()) + } + + // The guard must now have resolved the engine module's provider (lazy resolve + // fired during the registry's resolution), proving the accessor pathway works + // end-to-end rather than a nil snapshot. + if guard.Provider() != known { + t.Errorf("guard.Provider() not resolved to engine module provider after registry resolution: got %T", guard.Provider()) + } +} + +// TestProviderRegistryNilAccessorNoPanic proves the wiring-time nil-accessor path +// (no SecretGuard registered, or guard absent) degrades gracefully: the registry +// builds without panic and a SecretName provider resolves with an empty API key +// (existing behavior — no secret resolution, but no panic either). +func TestProviderRegistryNilAccessorNoPanic(t *testing.T) { + // NewProviderRegistry with a nil accessor must not panic; it's internally + // normalized to func() secrets.Provider { return nil }. + reg := NewProviderRegistry(setupTestDB(t), nil) + + // A SecretName provider resolves with an empty key (nil accessor → sp == nil + // → secret resolution skipped). This mirrors the pre-fix nil-snapshot + // behavior for the no-vault path. + _, err := reg.db.Exec(`INSERT INTO llm_providers (id, alias, type, secret_name) + VALUES ('p1', 'no-vault', 'mock', 'SOME_SECRET')`) + if err != nil { + t.Fatalf("insert: %v", err) + } + + p, err := reg.GetByAlias(context.Background(), "no-vault") + if err != nil { + t.Fatalf("GetByAlias with nil accessor: %v", err) + } + if p == nil { + t.Fatal("expected non-nil mock provider despite nil accessor") + } + if p.Name() != "mock" { + t.Errorf("expected mock provider, got %q", p.Name()) + } +} + +// TestProviderRegistryUpdateSecretsProviderOverridesLazy proves the runtime +// hot-swap (UpdateSecretsProvider, used by step.vault_config) takes precedence +// over the lazy guard accessor: after the swap, resolution uses the new provider +// and the lazy accessor is no longer consulted. +func TestProviderRegistryUpdateSecretsProviderOverridesLazy(t *testing.T) { + const vaultKey = "vault" + // Lazy-guard-backed provider would resolve "K" -> "lazy-val". + lazyProvider := &mockSecretsProvider{secrets: map[string]string{"K": "lazy-val"}} + vaultMod := &startableVaultModule{} + vaultMod.Start(lazyProvider) + app := &lazyStubApp{services: map[string]any{vaultKey: vaultMod}} + + guard := NewSecretGuard(nil, "") + guard.AttachLazyVault(app, vaultKey) + reg := NewProviderRegistry(setupTestDB(t), guard.Provider) + + // A factory that records the resolved apiKey, so we can assert WHICH provider + // supplied the secret (hot-swap vs lazy). + var resolvedKey string + reg.factories["recording"] = func(_ context.Context, apiKey string, _ LLMProviderConfig) (provider.Provider, error) { + resolvedKey = apiKey + return &mockProvider{responses: []string{"ok"}}, nil + } + + _, err := reg.db.Exec(`INSERT INTO llm_providers (id, alias, type, secret_name) + VALUES ('p1', 'swap-test', 'recording', 'K')`) + if err != nil { + t.Fatalf("insert: %v", err) + } + + // Hot-swap to a provider that resolves "K" -> "hotswap-val". + hotSwap := &mockSecretsProvider{secrets: map[string]string{"K": "hotswap-val"}} + reg.UpdateSecretsProvider(hotSwap) + + if _, err := reg.GetByAlias(context.Background(), "swap-test"); err != nil { + t.Fatalf("GetByAlias after hot-swap: %v", err) + } + + // The registry must have resolved the secret from the HOT-SWAP provider, + // proving it took precedence over the lazy guard accessor. + if resolvedKey != "hotswap-val" { + t.Errorf("hot-swap did not take precedence: resolved key %q, want %q", resolvedKey, "hotswap-val") + } +}