diff --git a/orchestrator/plugin.go b/orchestrator/plugin.go index 597d294..5def3db 100644 --- a/orchestrator/plugin.go +++ b/orchestrator/plugin.go @@ -42,7 +42,7 @@ func New() *RatchetPlugin { Description: "Ratchet autonomous agent orchestration plugin", ModuleTypes: []string{"agent.provider", "ratchet.sse_hub", "ratchet.scheduler", "ratchet.mcp_client", "ratchet.mcp_server", "authz.casbin", "agent.guardrails"}, StepTypes: []string{"step.agent_execute", "step.provider_test", "step.provider_models", "step.model_pull", "step.workspace_init", "step.container_control", "step.mcp_reload", "step.approval_resolve", "step.webhook_process", "step.security_audit", "step.test_interact", "step.human_request_resolve", "step.memory_extract", "step.blackboard_post", "step.blackboard_read", "step.self_improve_validate", "step.self_improve_diff", "step.self_improve_deploy", "step.lsp_diagnose"}, - WiringHooks: []string{"agent.provider_registry", "ratchet.sse_route_registration", "ratchet.mcp_server_route_registration", "ratchet.db_init", "ratchet.auth_token", "ratchet.secrets_guard", "ratchet.provider_registry", "ratchet.tool_policy_engine", "ratchet.sub_agent_manager", "ratchet.tool_registry", "ratchet.container_manager", "ratchet.transcript_recorder", "ratchet.skill_manager", "ratchet.approval_manager", "ratchet.human_request_manager", "ratchet.webhook_manager", "ratchet.security_auditor", "ratchet.browser_manager", "ratchet.test_interaction", "ratchet.blackboard", "ratchet.mcp_tools_wiring"}, + WiringHooks: []string{"agent.provider_registry", "ratchet.sse_route_registration", "ratchet.mcp_server_route_registration", "ratchet.db_init", "ratchet.auth_token", "ratchet.secrets_resolver", "ratchet.provider_registry", "ratchet.tool_policy_engine", "ratchet.sub_agent_manager", "ratchet.tool_registry", "ratchet.container_manager", "ratchet.transcript_recorder", "ratchet.skill_manager", "ratchet.approval_manager", "ratchet.human_request_manager", "ratchet.webhook_manager", "ratchet.security_auditor", "ratchet.browser_manager", "ratchet.test_interaction", "ratchet.blackboard", "ratchet.mcp_tools_wiring"}, }, }, } @@ -116,7 +116,6 @@ func (p *RatchetPlugin) WiringHooks() []plugin.WiringHook { mcpServerRouteHook(), dbInitHook(), authTokenHook(), - secretsGuardHook(), secretsResolverHook(), providerRegistryHook(), toolPolicyEngineHook(), @@ -181,81 +180,19 @@ func (p *RatchetPlugin) ModuleSchemas() []*schema.ModuleSchema { } } -// secretsGuardHook creates a SecretGuard and registers it in the service registry. +// secretsResolverHook builds the secretService composite (engine +// *secrets.Redactor + *secretsHolder) and registers it under the KEPT service +// key "ratchet-secret-guard". // -// 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. +// This is the SecretGuard dismantle (plan: 2026-06-30-secretguard-dismantle- +// shared-redactor; design v4; ADR 0057). The composite replaces the deleted +// SecretGuard as the registered service. The key is KEPT (D-KEEP-KEY) so the +// repo-root package-agent path — which resolves the service via alias lists and +// type-asserts to executor.SecretRedactor / interface{ Provider() secrets.Provider } +// — keeps working unchanged (D-COMPOSITE-SERVICE; no root-file edit). The +// composite satisfies both ifaces (asserted in secret_service.go). // -// 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 { - // No inline provider construction: the engine secrets.vault module - // owns the backend. The guard resolves it lazily on first redaction. - guard := NewSecretGuard(nil, "") - - // Arm lazy resolution against the engine secrets.vault module. - vaultModuleKey := os.Getenv("RATCHET_VAULT_MODULE") - if vaultModuleKey == "" { - vaultModuleKey = "vault" - } - guard.AttachLazyVault(app, vaultModuleKey) - - ctx := context.Background() - - // 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) - } - - // 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_") { - parts := strings.SplitN(env, "=", 2) - name := strings.TrimPrefix(parts[0], "RATCHET_") - if val, err := envProvider.Get(ctx, name); err == nil && val != "" { - guard.AddKnownSecret(name, val) - } - } - } - - app.Logger().Info("secrets guard armed (lazy-resolve engine vault module)", "vault_module", vaultModuleKey) - - // Register in service registry (the (R) steps resolve this). - _ = app.RegisterService("ratchet-secret-guard", guard) - return nil - }, - } -} - -// secretsResolverHook builds the new secretService composite (engine -// *secrets.Redactor + *secretsHolder) and registers it under the TEMPORARY key -// "ratchet-secret-guard-v2". -// -// This is the additive companion to secretsGuardHook (which still registers the -// legacy *SecretGuard under the live key "ratchet-secret-guard"). It is the -// second shot of the SecretGuard dismantle (plan: 2026-06-30-secretguard- -// dismantle-shared-redactor; design v4; ADR 0057): the composite is built + -// registered here so it can be exercised in isolation, and the TEMP key keeps -// it from colliding with the live SecretGuard registration. A follow-up shot -// rewires consumers onto the composite and flips the key back to -// "ratchet-secret-guard" (the kept key — D-KEEP-KEY), then deletes SecretGuard. -// -// P13 (two-source loading, mirrors secretsGuardHook exactly): +// P13 (two-source loading): // - VAULT_TOKEN: if a saved vault-config is present its token is registered // for redaction (the token must never leak into LLM output even though the // engine module owns the connection). @@ -263,7 +200,8 @@ func secretsGuardHook() plugin.WiringHook { // // D19 (lazy-resolve) is preserved by the holder: it is constructed with the app // + vaultKey, and resolves the engine secrets.vault module's Provider on first -// redaction/accessor call post-Start (wiring hooks run pre-Start). +// redaction/accessor call post-Start (wiring hooks run pre-Start). A single +// sync.Once makes resolve+arm atomic (D6). func secretsResolverHook() plugin.WiringHook { return plugin.WiringHook{ Name: "ratchet.secrets_resolver", @@ -303,11 +241,10 @@ func secretsResolverHook() plugin.WiringHook { app.Logger().Info("secrets resolver armed (composite; lazy-resolve engine vault module)", "vault_module", vaultModuleKey) svc := &secretService{redactor: redactor, holder: holder} - // TEMP key: the composite is registered alongside (not replacing) the - // live SecretGuard so this shot stays additive + revertible. The key - // flips to "ratchet-secret-guard" in a follow-up shot once consumers - // are rewired onto the composite. - _ = app.RegisterService("ratchet-secret-guard-v2", svc) + // KEPT key "ratchet-secret-guard" (D-KEEP-KEY): the repo-root package-agent + // path resolves the service here via alias lists; renaming would silently + // break redaction + API-key resolution on the live root-agent path. + _ = app.RegisterService("ratchet-secret-guard", svc) return nil }, } @@ -330,18 +267,18 @@ func providerRegistryHook() plugin.WiringHook { return nil // no DB, skip } - // Get secrets provider ACCESSOR from SecretGuard. + // Get secrets provider ACCESSOR from the secretService composite. // - // We pass the guard.Provider METHOD VALUE (not the call result) because + // We pass the holder.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 + // holder 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 { - spAccessor = guard.Provider + if ssvc, ok := svc.(*secretService); ok { + spAccessor = ssvc.Holder().Provider } } @@ -551,13 +488,13 @@ func transcriptRecorderHook() plugin.WiringHook { return nil // no DB, skip } - // Get SecretGuard (optional) - var guard *SecretGuard + // Get the secretService composite (optional) for transcript redaction. + var ssvc *secretService if svc, ok := app.SvcRegistry()["ratchet-secret-guard"]; ok { - guard, _ = svc.(*SecretGuard) + ssvc, _ = svc.(*secretService) } - recorder := NewTranscriptRecorder(db, guard) + recorder := NewTranscriptRecorder(db, ssvc) _ = app.RegisterService("ratchet-transcript-recorder", recorder) return nil }, @@ -640,12 +577,7 @@ func webhookManagerHook() plugin.WiringHook { return nil // no DB, skip } - var guard *SecretGuard - if svc, ok := app.SvcRegistry()["ratchet-secret-guard"]; ok { - guard, _ = svc.(*SecretGuard) - } - - wm := NewWebhookManager(db, guard) + wm := NewWebhookManager(db) _ = app.RegisterService("ratchet-webhook-manager", wm) return nil }, diff --git a/orchestrator/provider_registry.go b/orchestrator/provider_registry.go index 081568d..0582e19 100644 --- a/orchestrator/provider_registry.go +++ b/orchestrator/provider_registry.go @@ -46,10 +46,11 @@ type ProviderFactory func(ctx context.Context, apiKey string, cfg LLMProviderCon // 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. +// (the secretService composite's secretsHolder 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-swaps the accessor for +// the runtime vault-config path. type ProviderRegistry struct { mu sync.RWMutex db *sql.DB @@ -61,12 +62,13 @@ type ProviderRegistry struct { // NewProviderRegistry creates a new ProviderRegistry with built-in factories registered. // -// 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). +// secretsProvider is a lazy accessor (typically secretService.Holder().Provider) +// — it is invoked on demand at provider-resolution time (post-Start), NOT +// snapshotted at wiring time. This is required because the secretsHolder +// 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 } @@ -248,7 +250,7 @@ func (r *ProviderRegistry) createAndCache(ctx context.Context, alias string, cfg r.mu.RUnlock() // Resolve API key from secrets — invoke the lazy accessor (post-Start) - // so the SecretGuard's lazy-resolve fires at provider-resolution time, + // so the secretsHolder's lazy-resolve fires at provider-resolution time, // not the wiring-time nil snapshot. var apiKey string r.mu.RLock() diff --git a/orchestrator/provider_registry_test.go b/orchestrator/provider_registry_test.go index a5ebb5f..5a6f628 100644 --- a/orchestrator/provider_registry_test.go +++ b/orchestrator/provider_registry_test.go @@ -370,3 +370,149 @@ func TestProviderRegistryChatMock(t *testing.T) { t.Error("expected non-empty response") } } + +// --------------------------------------------------------------------------- +// Lazy-accessor + hot-swap registry tests (ported from secret_guard_lazy_test.go +// when SecretGuard was deleted; M3). These prove the ProviderRegistry resolves a +// vault-backed secret via the LAZY holder.Provider accessor (the method value +// secretsResolverHook wires, not a nil wiring-time snapshot) and that runtime +// hot-swap via UpdateSecretsProvider takes precedence over the lazy accessor. +// The accessor source changed from SecretGuard.Provider to secretsHolder.Provider; +// the registry behavior under test is unchanged. +// --------------------------------------------------------------------------- + +// TestProviderRegistryResolvesSecretViaLazyHolder proves the ProviderRegistry +// resolves a vault-backed secret via the LAZY holder.Provider accessor (the +// method value providerRegistryHook wires), 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 TestProviderRegistryResolvesSecretViaLazyHolder(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 holder exactly as secretsResolverHook does: no pre-set + // provider, armed for lazy resolution against the engine module. + holder := &secretsHolder{app: app, vaultKey: vaultKey, redactor: secrets.NewRedactor()} + + // The registry is built with the holder.Provider METHOD VALUE (lazy + // accessor), exactly as providerRegistryHook wires it. It must NOT snapshot + // nil here. + reg := NewProviderRegistry(setupTestDB(t), holder.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 holder'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. + p, err := reg.GetByAlias(context.Background(), "vaulted-claude") + if err != nil { + t.Fatalf("GetByAlias via lazy holder: %v", err) + } + if p == nil { + t.Fatal("expected non-nil provider resolved via lazy holder") + } + if p.Name() != "anthropic" { + t.Errorf("expected anthropic provider, got %q", p.Name()) + } + + // The holder 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 holder.Provider() != known { + t.Errorf("holder.Provider() not resolved to engine module provider after registry resolution: got %T", holder.Provider()) + } +} + +// TestProviderRegistryNilAccessorNoPanic proves the wiring-time nil-accessor +// path (no secretService registered, or holder unresolved) 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 on the ProviderRegistry) takes precedence +// over the lazy holder 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-holder-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}} + + holder := &secretsHolder{app: app, vaultKey: vaultKey, redactor: secrets.NewRedactor()} + reg := NewProviderRegistry(setupTestDB(t), holder.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 holder accessor. + if resolvedKey != "hotswap-val" { + t.Errorf("hot-swap did not take precedence: resolved key %q, want %q", resolvedKey, "hotswap-val") + } +} diff --git a/orchestrator/ratchetplugin_test.go b/orchestrator/ratchetplugin_test.go index 4d2bc8c..f3ed4d3 100644 --- a/orchestrator/ratchetplugin_test.go +++ b/orchestrator/ratchetplugin_test.go @@ -137,229 +137,16 @@ func createAllTables(t *testing.T, db *sql.DB) { } // --------------------------------------------------------------------------- -// SecretGuard tests +// SecretGuard + secretsGuardHook tests were here. Both were deleted in the +// SecretGuard dismantle (the secretService composite + secretsResolverHook +// replace them; ADR 0057). The equivalent coverage now lives in: +// - secret_service_test.go — composite Redact/CheckAndRedact/hot-swap/ +// lazy-resolve/store-then-arm/root-path-alias +// - secrets_resolver_hook_test.go — secretsResolverHook P13 + lazy-resolve +// - provider_registry_test.go — lazy-holder accessor + hot-swap precedence +// - workflow/secrets/redactor_test.go (engine) — value-scan redaction + race // --------------------------------------------------------------------------- -func TestNewSecretGuard(t *testing.T) { - p := &mockSecretsProvider{secrets: map[string]string{}} - sg := NewSecretGuard(p, "file") - if sg == nil { - t.Fatal("NewSecretGuard returned nil") - return - } - if sg.provider == nil { - t.Fatal("provider is nil") - } - if sg.knownValues == nil { - t.Fatal("knownValues map is nil") - } - if sg.BackendName() != "file" { - t.Errorf("BackendName: got %q, want %q", sg.BackendName(), "file") - } -} - -func TestSecretGuard_LoadSecrets(t *testing.T) { - p := &mockSecretsProvider{secrets: map[string]string{ - "API_KEY": "sk-abc123", - "DB_PASS": "super-secret", - "NO_VALUE": "", - }} - sg := NewSecretGuard(p, "test") - ctx := context.Background() - - err := sg.LoadSecrets(ctx, []string{"API_KEY", "DB_PASS", "NO_VALUE", "MISSING"}) - if err != nil { - t.Fatalf("LoadSecrets: %v", err) - } - - // Redact should replace known values - text := "key is sk-abc123 and password is super-secret" - redacted := sg.Redact(text) - if !strings.Contains(redacted, "[REDACTED:API_KEY]") { - t.Errorf("expected [REDACTED:API_KEY], got %q", redacted) - } - if !strings.Contains(redacted, "[REDACTED:DB_PASS]") { - t.Errorf("expected [REDACTED:DB_PASS], got %q", redacted) - } - if strings.Contains(redacted, "sk-abc123") { - t.Errorf("secret value still present in redacted text") - } -} - -func TestSecretGuard_LoadAllSecrets(t *testing.T) { - p := &mockSecretsProvider{secrets: map[string]string{ - "TOKEN": "tok-xyz", - }} - sg := NewSecretGuard(p, "test") - - err := sg.LoadAllSecrets(context.Background()) - if err != nil { - t.Fatalf("LoadAllSecrets: %v", err) - } - - redacted := sg.Redact("my token is tok-xyz") - if !strings.Contains(redacted, "[REDACTED:TOKEN]") { - t.Errorf("expected [REDACTED:TOKEN], got %q", redacted) - } -} - -func TestSecretGuard_LoadAllSecrets_NilProvider(t *testing.T) { - sg := &SecretGuard{knownValues: make(map[string]string)} - err := sg.LoadAllSecrets(context.Background()) - if err != nil { - t.Fatalf("LoadAllSecrets with nil provider should not error, got: %v", err) - } -} - -func TestSecretGuard_CheckAndRedact(t *testing.T) { - p := &mockSecretsProvider{secrets: map[string]string{"KEY": "secret123"}} - sg := NewSecretGuard(p, "test") - _ = sg.LoadSecrets(context.Background(), []string{"KEY"}) - - msg := &provider.Message{Content: "the secret is secret123"} - changed := sg.CheckAndRedact(msg) - if !changed { - t.Error("expected CheckAndRedact to return true") - } - if !strings.Contains(msg.Content, "[REDACTED:KEY]") { - t.Errorf("expected redacted content, got %q", msg.Content) - } -} - -func TestSecretGuard_CheckAndRedact_NoChange(t *testing.T) { - p := &mockSecretsProvider{secrets: map[string]string{"KEY": "secret123"}} - sg := NewSecretGuard(p, "test") - _ = sg.LoadSecrets(context.Background(), []string{"KEY"}) - - msg := &provider.Message{Content: "nothing to redact here"} - changed := sg.CheckAndRedact(msg) - if changed { - t.Error("expected CheckAndRedact to return false when no redaction occurs") - } -} - -func TestSecretGuard_Redact_NoSecretsLoaded(t *testing.T) { - sg := NewSecretGuard(&mockSecretsProvider{secrets: map[string]string{}}, "test") - text := "nothing secret here" - if got := sg.Redact(text); got != text { - t.Errorf("expected unchanged text, got %q", got) - } -} - -func TestSecretGuard_ConcurrentAccess(t *testing.T) { - p := &mockSecretsProvider{secrets: map[string]string{ - "A": "val-a", - "B": "val-b", - "C": "val-c", - }} - sg := NewSecretGuard(p, "test") - ctx := context.Background() - - var wg sync.WaitGroup - // Load secrets concurrently - for i := 0; i < 10; i++ { - wg.Add(1) - go func() { - defer wg.Done() - _ = sg.LoadSecrets(ctx, []string{"A", "B", "C"}) - }() - } - // Redact concurrently - for i := 0; i < 10; i++ { - wg.Add(1) - go func() { - defer wg.Done() - _ = sg.Redact("contains val-a and val-b") - }() - } - 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 // --------------------------------------------------------------------------- @@ -545,8 +332,7 @@ func TestTranscriptRecorder_SecretRedaction(t *testing.T) { initTranscriptsTable(t, db) p := &mockSecretsProvider{secrets: map[string]string{"API_KEY": "sk-secret-val"}} - sg := NewSecretGuard(p, "test") - _ = sg.LoadAllSecrets(context.Background()) + sg := newTestSecretService(p) rec := NewTranscriptRecorder(db, sg) ctx := context.Background() @@ -949,7 +735,7 @@ func TestAgentExecuteStep_SimpleCompletion(t *testing.T) { } // Set up guard - sg := NewSecretGuard(&mockSecretsProvider{secrets: map[string]string{}}, "test") + sg := newTestSecretService(&mockSecretsProvider{secrets: map[string]string{}}) // Set up recorder rec := NewTranscriptRecorder(db, sg) @@ -1017,8 +803,7 @@ func TestAgentExecuteStep_SecretRedaction(t *testing.T) { mp := &mockProvider{responses: []string{"Done."}} providerMod := &AIProviderModule{name: "ratchet-ai", provider: mp} - sg := NewSecretGuard(&mockSecretsProvider{secrets: map[string]string{"PASS": "my-secret-pass"}}, "test") - _ = sg.LoadAllSecrets(context.Background()) + sg := newTestSecretService(&mockSecretsProvider{secrets: map[string]string{"PASS": "my-secret-pass"}}) rec := NewTranscriptRecorder(db, sg) app := newMockApp() @@ -1137,8 +922,8 @@ func TestPlugin_WiringHooks(t *testing.T) { p := New() hooks := p.WiringHooks() - if len(hooks) != 22 { - t.Fatalf("expected 22 wiring hooks, got %d", len(hooks)) + if len(hooks) != 21 { + t.Fatalf("expected 21 wiring hooks, got %d", len(hooks)) } expectedNames := map[string]bool{ @@ -1147,7 +932,6 @@ func TestPlugin_WiringHooks(t *testing.T) { "ratchet.mcp_server_route_registration": false, "ratchet.db_init": false, "ratchet.auth_token": false, - "ratchet.secrets_guard": false, "ratchet.secrets_resolver": false, "ratchet.provider_registry": false, "ratchet.tool_registry": false, @@ -1177,11 +961,10 @@ func TestPlugin_WiringHooks(t *testing.T) { } } - // Verify priorities: db_init(100) > auth_token(90) > secrets_guard(85) > secrets_resolver(84) > provider_registry(83) > tool_registry(80) > transcript_recorder(75) + // Verify priorities: db_init(100) > auth_token(90) > secrets_resolver(84) > provider_registry(83) > tool_registry(80) > transcript_recorder(75) expectedPriorities := map[string]int{ "ratchet.db_init": 100, "ratchet.auth_token": 90, - "ratchet.secrets_guard": 85, "ratchet.secrets_resolver": 84, "ratchet.provider_registry": 83, "ratchet.tool_registry": 80, @@ -1277,7 +1060,7 @@ func TestAgentExecuteStep_WorkspaceInjectedIntoContext(t *testing.T) { // toolCallOnceProvider emits a single tool call on turn 1, then "Done." on turn 2. mp := &toolCallOnceProvider{toolName: "capture_workspace"} providerMod := &AIProviderModule{name: "ratchet-ai", provider: mp} - sg := NewSecretGuard(&mockSecretsProvider{secrets: map[string]string{}}, "test") + sg := newTestSecretService(&mockSecretsProvider{secrets: map[string]string{}}) rec := NewTranscriptRecorder(db, sg) tr := NewToolRegistry() diff --git a/orchestrator/secret_guard.go b/orchestrator/secret_guard.go deleted file mode 100644 index 9d7188e..0000000 --- a/orchestrator/secret_guard.go +++ /dev/null @@ -1,174 +0,0 @@ -package orchestrator - -import ( - "context" - "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 { - return &SecretGuard{ - knownValues: make(map[string]string), - provider: p, - backendName: backend, - } -} - -// 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. -func (sg *SecretGuard) SetProvider(p secrets.Provider, backend string) { - // Pre-load secrets from the new provider outside the lock - newValues := make(map[string]string) - if p != nil { - ctx := context.Background() - if names, err := p.List(ctx); err == nil { - for _, name := range names { - if val, err := p.Get(ctx, name); err == nil && val != "" { - newValues[val] = name - } - } - } - } - - // Atomic swap under write lock - sg.mu.Lock() - sg.provider = p - sg.backendName = backend - sg.knownValues = newValues - sg.mu.Unlock() -} - -// BackendName returns the name of the current backend. -func (sg *SecretGuard) BackendName() string { - sg.mu.RLock() - defer sg.mu.RUnlock() - return sg.backendName -} - -// LoadSecrets loads secret values from the provider for the given keys. -func (sg *SecretGuard) LoadSecrets(ctx context.Context, names []string) error { - sg.mu.Lock() - defer sg.mu.Unlock() - for _, name := range names { - val, err := sg.provider.Get(ctx, name) - if err != nil { - continue // skip secrets that don't exist - } - if val != "" { - sg.knownValues[val] = name - } - } - return nil -} - -// LoadAllSecrets loads all available secrets from the provider. -func (sg *SecretGuard) LoadAllSecrets(ctx context.Context) error { - if sg.provider == nil { - return nil - } - names, err := sg.provider.List(ctx) - if err != nil { - return err - } - return sg.LoadSecrets(ctx, names) -} - -// 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 { - if strings.Contains(text, val) { - text = strings.ReplaceAll(text, val, "[REDACTED:"+name+"]") - } - } - return text -} - -// CheckAndRedact redacts secret values in a message. Returns true if redaction occurred. -func (sg *SecretGuard) CheckAndRedact(msg *provider.Message) bool { - original := msg.Content - msg.Content = sg.Redact(msg.Content) - return msg.Content != original -} diff --git a/orchestrator/secret_guard_ext.go b/orchestrator/secret_guard_ext.go deleted file mode 100644 index 5430397..0000000 --- a/orchestrator/secret_guard_ext.go +++ /dev/null @@ -1,28 +0,0 @@ -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 -} - -// AddKnownSecret adds a secret value to the guard's redaction list. -func (sg *SecretGuard) AddKnownSecret(name, value string) { - if value == "" { - return - } - sg.mu.Lock() - defer sg.mu.Unlock() - sg.knownValues[value] = name -} diff --git a/orchestrator/secret_guard_lazy_test.go b/orchestrator/secret_guard_lazy_test.go deleted file mode 100644 index 7db46c8..0000000 --- a/orchestrator/secret_guard_lazy_test.go +++ /dev/null @@ -1,322 +0,0 @@ -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 on the ProviderRegistry) 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") - } -} diff --git a/orchestrator/secret_service.go b/orchestrator/secret_service.go index fe1582d..2994176 100644 --- a/orchestrator/secret_service.go +++ b/orchestrator/secret_service.go @@ -10,11 +10,18 @@ import ( "github.com/GoCodeAlone/workflow/secrets" ) -// vaultProvider is the structural interface the lazy-resolver type-asserts on -// when looking up the engine secrets.vault module in the service registry. It -// is declared in orchestrator/secret_guard.go (the legacy SecretGuard file) and -// reused here unchanged; it will collapse onto this file when SecretGuard is -// removed in a follow-up shot. +// vaultProvider exposes the Provider() accessor the lazy-resolver type-asserts +// on when looking up the engine secrets.vault module in the service registry. +// 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). +// +// (Moved here from secret_guard.go when SecretGuard was deleted — the lazy- +// resolver in *secretsHolder is the sole remaining user.) +type vaultProvider interface { + Provider() secrets.Provider +} // secretsHolder is the live, hot-swappable slot for the engine secrets.vault // module's Provider. It preserves the D19 lazy-resolve lifecycle (wiring hooks diff --git a/orchestrator/secret_service_test.go b/orchestrator/secret_service_test.go index 35a4b9f..114f6ab 100644 --- a/orchestrator/secret_service_test.go +++ b/orchestrator/secret_service_test.go @@ -1,14 +1,63 @@ package orchestrator import ( + "context" + "fmt" "strings" "sync" "testing" + "github.com/GoCodeAlone/modular" + "github.com/GoCodeAlone/workflow-plugin-agent/executor" "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. +// (Moved here from secret_guard_lazy_test.go when SecretGuard was deleted.) +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. (Moved here from secret_guard_lazy_test.go.) +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{} } + +// 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. +// (Moved here from secret_guard_lazy_test.go.) +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 } + // These tests cover the new secretsHolder + secretService composite (the // additive replacement for SecretGuard). They PORT the D19 lazy-resolve + // hot-swap + override-precedence regression coverage from @@ -220,3 +269,142 @@ func TestSecretService_Redact_DelegatesAndArms(t *testing.T) { t.Errorf("CheckAndRedact left secret value in msg.Content: %q", msg.Content) } } + +// newTestSecretService builds a *secretService composite for test injection +// (replacing the former NewSecretGuard construction). The Redactor is armed +// from the supplied provider (mirroring what LoadFromProvider does at runtime), +// so a known value supplied by the provider is redacted. Pass nil to get the +// env-only path (no vault provider). +func newTestSecretService(p secrets.Provider) *secretService { + redactor := secrets.NewRedactor() + holder := &secretsHolder{redactor: redactor} + svc := &secretService{redactor: redactor, holder: holder} + if p != nil { + holder.SetProvider(p) // arms the Redactor via LoadFromProvider + } + return svc +} + +// TestSecretService_StoreThenRedact (D3 non-regression) proves the store-then-arm +// path preserved by the autoStoreSecret rewire: after a token is stored via +// Provider.Set AND armed via Redactor().AddValue, a subsequent Redact masks it. +// Dropping the AddValue call (the regression this guards) would silently leak +// the just-stored token in the next redaction pass — the failure class the +// redactor exists to prevent. +func TestSecretService_StoreThenRedact(t *testing.T) { + // A settable vault provider whose store records the Set so Get can return it + // later (mirrors step_human_request.autoStoreSecret: Provider().Set then arm). + // mockSecretsProvider.Set returns ErrUnsupported, so we use a local mutable + // provider here. + vault := &settableSecretsProvider{secrets: map[string]string{}} + svc := newTestSecretService(vault) + + const secretName = "FRESH_API_KEY" + const token = "sk-just-stored-by-human-request" + + // Store the token via the provider (autoStoreSecret does Provider().Set). + if err := svc.Provider().Set(context.Background(), secretName, token); err != nil { + t.Fatalf("Provider.Set: %v", err) + } + // Arm the redactor with the just-stored token (autoStoreSecret does + // Redactor().AddValue after Set — D3). This is the load-bearing call. + svc.Redactor().AddValue(secretName, token) + + // A subsequent Redact MUST mask the token. If AddValue had been dropped, the + // token would leak here (the silent-leak regression). + got := svc.Redact("leaked token: sk-just-stored-by-human-request in output") + if strings.Contains(got, token) { + t.Errorf("D3 regression: just-stored token leaked after Redact: %q", got) + } + if !strings.Contains(got, "[REDACTED:"+secretName+"]") { + t.Errorf("D3: expected [REDACTED:%s], got %q", secretName, got) + } +} + +// settableSecretsProvider is a secrets.Provider whose Set mutates the map (unlike +// mockSecretsProvider, which returns ErrUnsupported). Used by the D3 store-then- +// redact test to exercise the autoStoreSecret Provider().Set path. +type settableSecretsProvider struct { + secrets map[string]string +} + +func (m *settableSecretsProvider) Name() string { return "settable" } +func (m *settableSecretsProvider) Get(_ context.Context, name string) (string, error) { + v, ok := m.secrets[name] + if !ok { + return "", fmt.Errorf("secret %q not found", name) + } + return v, nil +} +func (m *settableSecretsProvider) Set(_ context.Context, name, value string) error { + m.secrets[name] = value + return nil +} +func (m *settableSecretsProvider) Delete(_ context.Context, name string) error { + delete(m.secrets, name) + return nil +} +func (m *settableSecretsProvider) List(_ context.Context) ([]string, error) { + names := make([]string, 0, len(m.secrets)) + for k := range m.secrets { + names = append(names, k) + } + return names, nil +} + +// TestSecretService_RootPathAliasListsResolveComposite (Task 10 / D10 +// non-regression) proves the repo-root package-agent path resolves the +// *secretService composite under the KEPT key "ratchet-secret-guard" via BOTH +// alias lists + type-assertions it uses. This is the runtime mirror of the +// compile-time iface assertions in secret_service.go: a compile-assert can't +// catch a key-typo or registration-ordering bug, so we exercise the real +// alias-list lookup against a fake app whose SvcRegistry returns the composite +// under the kept key. +// +// Root path consumers (repo-root package agent, UNCHANGED by this shot): +// - step_agent_execute.go: alias list ["agent-secret-guard","ratchet-secret-guard"] +// -> type-assert executor.SecretRedactor +// - provider_registry.go: alias list ["ratchet-secret-guard","agent-secret-guard","secret-guard"] +// -> type-assert interface{ Provider() secrets.Provider } +func TestSecretService_RootPathAliasListsResolveComposite(t *testing.T) { + svc := newTestSecretService(&mockSecretsProvider{secrets: map[string]string{"K": "v"}}) + // Fake app whose registry returns the composite under the KEPT key, exactly + // as secretsResolverHook registers it. lazyStubApp satisfies modular.Application. + app := &lazyStubApp{services: map[string]any{"ratchet-secret-guard": svc}} + + // Path 1: repo-root step_agent_execute.go alias list -> executor.SecretRedactor. + var resolvedRedactor executor.SecretRedactor + for _, name := range []string{"agent-secret-guard", "ratchet-secret-guard"} { + if r, ok := app.SvcRegistry()[name].(executor.SecretRedactor); ok { + resolvedRedactor = r + break + } + } + if resolvedRedactor == nil { + t.Fatal("D10: root path 1 (executor.SecretRedactor) did not resolve the composite under ratchet-secret-guard") + } + if resolvedRedactor != svc { + t.Errorf("D10: root path 1 resolved %T, want the registered *secretService", resolvedRedactor) + } + + // Path 2: repo-root provider_registry.go alias list -> interface{ Provider() secrets.Provider }. + var resolvedProvider interface{ Provider() secrets.Provider } + for _, name := range []string{"ratchet-secret-guard", "agent-secret-guard", "secret-guard"} { + if p, ok := app.SvcRegistry()[name].(interface{ Provider() secrets.Provider }); ok { + resolvedProvider = p + break + } + } + if resolvedProvider == nil { + t.Fatal("D10: root path 2 (Provider accessor) did not resolve the composite under ratchet-secret-guard") + } + if resolvedProvider != svc { + t.Errorf("D10: root path 2 resolved %T, want the registered *secretService", resolvedProvider) + } + + // Both paths resolve the SAME composite, and the Provider accessor returns + // the wired provider (proving API-key resolution would work end-to-end). + if resolvedProvider.Provider() == nil { + t.Error("D10: resolved composite's Provider() is nil; want the wired mock provider") + } +} diff --git a/orchestrator/secrets_resolver_hook_test.go b/orchestrator/secrets_resolver_hook_test.go index 0de45c7..2c61ea7 100644 --- a/orchestrator/secrets_resolver_hook_test.go +++ b/orchestrator/secrets_resolver_hook_test.go @@ -10,16 +10,15 @@ import ( "github.com/GoCodeAlone/workflow/secrets" ) -// secrets_resolver_hook_test.go exercises the additive secretsResolverHook -// (Task 5). It proves the hook builds a *secretService composite (engine -// Redactor + secretsHolder), registers it under the TEMP key -// "ratchet-secret-guard-v2", and performs P13 two-source loading (VAULT_TOKEN + -// RATCHET_* env) into the Redactor so the values are redacted. The legacy -// secretsGuardHook / *SecretGuard registration under the live key -// "ratchet-secret-guard" is untouched (additive shot). +// secrets_resolver_hook_test.go exercises secretsResolverHook. It proves the +// hook builds a *secretService composite (engine Redactor + secretsHolder), +// registers it under the KEPT key "ratchet-secret-guard" (D-KEEP-KEY — the +// repo-root package-agent path resolves the service here via alias lists), and +// performs P13 two-source loading (VAULT_TOKEN + RATCHET_* env) into the +// Redactor so the values are redacted. // runSecretsResolverHook invokes secretsResolverHook against a fresh mockApp -// and returns the app + the registered *secretService (looked up under the TEMP +// and returns the app + the registered *secretService (looked up under the KEPT // key). t.Helper centralizes the lookup so each test asserts behavior, not // registration mechanics. func runSecretsResolverHook(t *testing.T) (*mockApp, *secretService) { @@ -29,22 +28,22 @@ func runSecretsResolverHook(t *testing.T) (*mockApp, *secretService) { if err := hook.Hook(app, nil); err != nil { t.Fatalf("secretsResolverHook returned error: %v", err) } - raw, ok := app.services["ratchet-secret-guard-v2"] + raw, ok := app.services["ratchet-secret-guard"] if !ok { - t.Fatal("secretService not registered under ratchet-secret-guard-v2") + t.Fatal("secretService not registered under ratchet-secret-guard") } svc, ok := raw.(*secretService) if !ok { - t.Fatalf("ratchet-secret-guard-v2 service = %T, want *secretService", raw) + t.Fatalf("ratchet-secret-guard service = %T, want *secretService", raw) } return app, svc } -// TestSecretsResolverHook_RegistersCompositeUnderTempKey proves the hook -// registers a *secretService (not *SecretGuard) under the TEMP key -// "ratchet-secret-guard-v2", and that the composite satisfies both root-path -// interfaces the consumers type-assert on. -func TestSecretsResolverHook_RegistersCompositeUnderTempKey(t *testing.T) { +// TestSecretsResolverHook_RegistersCompositeUnderKeptKey proves the hook +// registers a *secretService (not *SecretGuard) under the KEPT key +// "ratchet-secret-guard", and that the composite satisfies both root-path +// interfaces the consumers type-assert on (D10/D-COMPOSITE-SERVICE). +func TestSecretsResolverHook_RegistersCompositeUnderKeptKey(t *testing.T) { _, svc := runSecretsResolverHook(t) // Compile-time iface satisfaction is asserted in secret_service.go; this is @@ -52,12 +51,6 @@ func TestSecretsResolverHook_RegistersCompositeUnderTempKey(t *testing.T) { // satisfies both shapes. var _ executor.SecretRedactor = svc var _ interface{ Provider() secrets.Provider } = svc - - // Legacy key MUST remain free of the composite in this additive shot (the - // live SecretGuard owns it until the rewire shot). - if _, ok := newMockApp().services["ratchet-secret-guard"]; ok { - t.Fatal("precondition: fresh app should not have ratchet-secret-guard") - } } // TestSecretsResolverHook_LoadsRatchetEnvForRedaction proves P13: RATCHET_* env diff --git a/orchestrator/security_audit.go b/orchestrator/security_audit.go index 96c065e..abf24ed 100644 --- a/orchestrator/security_audit.go +++ b/orchestrator/security_audit.go @@ -690,7 +690,7 @@ func (c *SecretExposureCheck) Run(ctx context.Context) []AuditFinding { Severity: SeverityHigh, Title: fmt.Sprintf("Potential secret exposure in %d transcript(s)", count), Description: fmt.Sprintf("%d transcript entries contain patterns that look like credentials (API keys, tokens, passwords) and are not redacted.", count), - Remediation: "Enable secret redaction via the SecretGuard. Rotate any potentially exposed credentials immediately.", + Remediation: "Enable secret redaction (configure the secrets.vault module / RATCHET_* env). Rotate any potentially exposed credentials immediately.", }) } diff --git a/orchestrator/services.go b/orchestrator/services.go index 35c1c1f..fa2c85d 100644 --- a/orchestrator/services.go +++ b/orchestrator/services.go @@ -11,7 +11,6 @@ import ( "github.com/GoCodeAlone/workflow-plugin-agent/orchestrator/tools" "github.com/GoCodeAlone/workflow-plugin-agent/provider" "github.com/GoCodeAlone/workflow/module" - "github.com/GoCodeAlone/workflow/secrets" ) // This file defines the orchestrator-scoped service interfaces that the (R) @@ -146,45 +145,27 @@ func (a toolRegistryAdapter) SetPaginator(rp *ResponsePaginator) { } // --------------------------------------------------------------------------- -// SecretGuard +// secretService (was SecretGuard) // --------------------------------------------------------------------------- -// SecretGuardService is the orchestrator-scoped secret redaction / provider. +// The orchestrator-scoped secret redaction / provider service is the +// *secretService composite (engine *secrets.Redactor + *secretsHolder), +// registered under the kept key "ratchet-secret-guard". It is NOT an interface +// here: it is a concrete pointer type so consumers reach the composite directly +// (redaction delegates to the engine Redactor; provider access delegates to the +// holder). Absence (no hook ran / app==nil) is represented by a nil pointer; +// the two accessor consumers (step_human_request, step_webhook) nil-check +// Provider() rather than IsNull (D5/D13). // // Consumers: -// - step_human_request (TRULY-OPTIONAL — autoStoreSecret helper only). -// - step_webhook (TRULY-OPTIONAL — signature-verification secret lookup). -// - step_agent_execute (TRULY-OPTIONAL — CheckAndRedact/Redact on messages). -// -// Phase 3 (PR5): the bespoke step_secret_manage/vault_config consumers were -// removed; secret mutations now flow through the engine step.secret_set / -// step.secret_fetch + workflow-plugin-infra secret-admin steps. The Provider / -// LoadSecrets / LoadAllSecrets methods are retained for the lazy-resolve -// redaction path and the optional consumers above. -type SecretGuardService interface { - LoadSecrets(ctx context.Context, names []string) error - LoadAllSecrets(ctx context.Context) error - Redact(text string) string - CheckAndRedact(msg *provider.Message) bool - AddKnownSecret(name, value string) - // Provider returns the underlying secrets.Provider so consumers can - // Get/Set raw secret values (webhook signature-secret lookup, human-request - // token auto-store). Added in P2-T3: step_webhook + step_human_request both - // need direct secret access that the redaction-oriented methods don't cover. - Provider() secrets.Provider -} - -// NullSecretGuard is a no-op SecretGuardService. Redact returns its input -// unchanged; CheckAndRedact returns false (no redaction performed); the load -// methods are no-ops. -type NullSecretGuard struct{ nullBase } - -func (NullSecretGuard) LoadSecrets(_ context.Context, _ []string) error { return nil } -func (NullSecretGuard) LoadAllSecrets(_ context.Context) error { return nil } -func (NullSecretGuard) Redact(text string) string { return text } -func (NullSecretGuard) CheckAndRedact(_ *provider.Message) bool { return false } -func (NullSecretGuard) AddKnownSecret(_, _ string) {} -func (NullSecretGuard) Provider() secrets.Provider { return nil } +// - step_human_request (TRULY-OPTIONAL — autoStoreSecret helper; nil-checks +// Provider() before Get/Set, then arms the Redactor via AddValue — D3). +// - step_webhook (TRULY-OPTIONAL — signature-verification secret lookup; +// nil-checks Provider()). +// - step_agent_execute (CheckAndRedact/Redact on messages; the composite is +// always registered for a real app so no gate is needed — D13). +// - TranscriptRecorder (Redact on recorded content). +// - providerRegistryHook (holder.Provider method value for API-key resolution). // --------------------------------------------------------------------------- // ApprovalManager @@ -482,10 +463,16 @@ func (NullTranscript) Record(_ context.Context, _ TranscriptEntry) error { retur // application's service registry. Every service field is non-nil: an absent // service is represented by its Null default. Steps inspect IsNull(iface) to // distinguish "present" from "absent". DB is nil when "ratchet-db" is absent. +// +// SecretGuard is the exception: it is a concrete *secretService pointer (D14 — +// field name KEPT to avoid consumer churn; type changed from the deleted +// SecretGuardService iface). Absence is a nil pointer, NOT a Null default, so +// IsNull(svcs.SecretGuard) is false even when absent — accessor consumers +// nil-check Provider()/the pointer directly (D5/D13). type serviceBundle struct { Blackboard BlackboardService ToolRegistry ToolRegistryService - SecretGuard SecretGuardService + SecretGuard *secretService Approval ApprovalService HumanRequest HumanRequestService SubAgent SubAgentService @@ -524,7 +511,6 @@ func resolveServices(app modular.Application) serviceBundle { return serviceBundle{ Blackboard: NullBlackboard{}, ToolRegistry: NullToolRegistry{}, - SecretGuard: NullSecretGuard{}, Approval: NullApproval{}, HumanRequest: NullHumanRequest{}, SubAgent: NullSubAgent{}, @@ -539,7 +525,6 @@ func resolveServices(app modular.Application) serviceBundle { b := serviceBundle{ Blackboard: NullBlackboard{}, ToolRegistry: NullToolRegistry{}, - SecretGuard: NullSecretGuard{}, Approval: NullApproval{}, HumanRequest: NullHumanRequest{}, SubAgent: NullSubAgent{}, @@ -556,7 +541,7 @@ func resolveServices(app modular.Application) serviceBundle { if svc, ok := reg["ratchet-tool-registry"].(*ToolRegistry); ok && svc != nil { b.ToolRegistry = toolRegistryAdapter{tr: svc} } - if svc, ok := reg["ratchet-secret-guard"].(*SecretGuard); ok && svc != nil { + if svc, ok := reg["ratchet-secret-guard"].(*secretService); ok && svc != nil { b.SecretGuard = svc } if svc, ok := reg["ratchet-approval-manager"].(*ApprovalManager); ok && svc != nil { @@ -601,7 +586,6 @@ func (b serviceBundle) String() string { }{ {"blackboard", b.Blackboard}, {"tool_registry", b.ToolRegistry}, - {"secret_guard", b.SecretGuard}, {"approval", b.Approval}, {"human_request", b.HumanRequest}, {"sub_agent", b.SubAgent}, @@ -616,6 +600,11 @@ func (b serviceBundle) String() string { present++ } } + // SecretGuard is a concrete *secretService pointer (not a Null default), so + // IsNull can't detect its absence — count it via nil-check instead. + if b.SecretGuard != nil { + present++ + } db := "absent" if b.DB != nil { db = "present" @@ -626,11 +615,14 @@ func (b serviceBundle) String() string { // Compile-time assertions that the Null defaults satisfy their interfaces, // that the adapters satisfy theirs, and that the concrete structs that need no // adapter satisfy their interfaces directly. +// +// secretService is omitted here: it is a concrete pointer (not an interface +// satisfaction site) and its structural-interface satisfaction is asserted in +// secret_service.go (executor.SecretRedactor + interface{ Provider() }). var ( // Null defaults. _ BlackboardService = NullBlackboard{} _ ToolRegistryService = NullToolRegistry{} - _ SecretGuardService = NullSecretGuard{} _ ApprovalService = NullApproval{} _ HumanRequestService = NullHumanRequest{} _ SubAgentService = NullSubAgent{} @@ -646,7 +638,6 @@ var ( // Concrete structs that satisfy their interface directly (no adapter). _ BlackboardService = (*Blackboard)(nil) - _ SecretGuardService = (*SecretGuard)(nil) _ ApprovalService = (*ApprovalManager)(nil) _ HumanRequestService = (*HumanRequestManager)(nil) _ SubAgentService = (*SubAgentManager)(nil) diff --git a/orchestrator/services_test.go b/orchestrator/services_test.go index e064fb9..21ed2ce 100644 --- a/orchestrator/services_test.go +++ b/orchestrator/services_test.go @@ -7,7 +7,6 @@ import ( "testing" "github.com/GoCodeAlone/workflow-plugin-agent/orchestrator/tools" - "github.com/GoCodeAlone/workflow-plugin-agent/provider" "github.com/google/uuid" ) @@ -27,7 +26,7 @@ import ( func TestIsNull_NullDefaultsReportTrue(t *testing.T) { nulls := []any{ - NullBlackboard{}, NullToolRegistry{}, NullSecretGuard{}, NullApproval{}, + NullBlackboard{}, NullToolRegistry{}, NullApproval{}, NullHumanRequest{}, NullSubAgent{}, NullSkill{}, NullContainer{}, NullWebhook{}, NullMemoryStore{}, NullTranscript{}, } @@ -53,13 +52,13 @@ func TestIsNull_ConcreteAndAdapterReportFalse(t *testing.T) { concretes := []any{ NewBlackboard(db, nil), NewToolRegistry(), - NewSecretGuard(&mockSecretsProvider{secrets: map[string]string{}}, "test"), + newTestSecretService(&mockSecretsProvider{secrets: map[string]string{}}), NewApprovalManager(db), NewHumanRequestManager(db), NewSubAgentManager(db, 0, 0), NewSkillManager(db, ""), NewContainerManager(db), - NewWebhookManager(db, NewSecretGuard(&mockSecretsProvider{secrets: map[string]string{}}, "test")), + NewWebhookManager(db), NewMemoryStore(db), // Adapters wrap concrete structs → never Null. toolRegistryAdapter{tr: NewToolRegistry()}, @@ -112,28 +111,12 @@ func TestNullToolRegistry_AllNoOp(t *testing.T) { } // --------------------------------------------------------------------------- -// NullSecretGuard +// NullSecretGuard tests removed: NullSecretGuard was deleted in the SecretGuard +// dismantle (the secretService composite is concrete, not a Null default; +// absence is a nil pointer — D5/D13/D14). Redactor passthrough behavior is +// covered by secret_service_test.go (TestSecretService_Redact_DelegatesAndArms). // --------------------------------------------------------------------------- -func TestNullSecretGuard_RedactIsPassthrough(t *testing.T) { - n := NullSecretGuard{} - const in = "secret value xyz" - if out := n.Redact(in); out != in { - t.Errorf("NullSecretGuard.Redact = %q, want %q (passthrough)", out, in) - } - if n.CheckAndRedact(&provider.Message{Role: provider.RoleUser, Content: in}) { - t.Error("NullSecretGuard.CheckAndRedact = true, want false (no redaction)") - } - if err := n.LoadSecrets(context.Background(), []string{"x"}); err != nil { - t.Errorf("NullSecretGuard.LoadSecrets err = %v, want nil", err) - } - if err := n.LoadAllSecrets(context.Background()); err != nil { - t.Errorf("NullSecretGuard.LoadAllSecrets err = %v, want nil", err) - } - // AddKnownSecret must be a safe no-op (no panic, no state observable via the interface). - n.AddKnownSecret("k", "v") -} - // --------------------------------------------------------------------------- // NullApproval // --------------------------------------------------------------------------- @@ -381,9 +364,11 @@ func TestContainerAdapter_NotNull(t *testing.T) { // --------------------------------------------------------------------------- func TestServiceBundle_String_AllAbsent(t *testing.T) { - // When resolveServices hands back all Null defaults, String reports present=0. + // When resolveServices hands back all Null defaults (SecretGuard nil), String + // reports present=0. SecretGuard is a concrete *secretService pointer; its + // absence is the zero value (nil), not a Null default (D13/D14). b := serviceBundle{ - Blackboard: NullBlackboard{}, ToolRegistry: NullToolRegistry{}, SecretGuard: NullSecretGuard{}, + Blackboard: NullBlackboard{}, ToolRegistry: NullToolRegistry{}, Approval: NullApproval{}, HumanRequest: NullHumanRequest{}, SubAgent: NullSubAgent{}, Skill: NullSkill{}, Container: NullContainer{}, Webhook: NullWebhook{}, Memory: NullMemoryStore{}, Transcript: NullTranscript{}, @@ -402,7 +387,6 @@ func TestServiceBundle_String_AllAbsent(t *testing.T) { var ( _ BlackboardService = NullBlackboard{} _ ToolRegistryService = NullToolRegistry{} - _ SecretGuardService = NullSecretGuard{} _ ApprovalService = NullApproval{} _ HumanRequestService = NullHumanRequest{} _ SubAgentService = NullSubAgent{} diff --git a/orchestrator/step_agent_execute.go b/orchestrator/step_agent_execute.go index 9ae47e6..940e318 100644 --- a/orchestrator/step_agent_execute.go +++ b/orchestrator/step_agent_execute.go @@ -119,7 +119,6 @@ func (s *AgentExecuteStep) Execute(ctx context.Context, pc *module.PipelineConte // gracefully when tools/secrets/transcripts/etc. are absent. svcs := resolveServices(s.app) toolRegistry := svcs.ToolRegistry - guard := svcs.SecretGuard recorder := svcs.Transcript containerSvc := svcs.Container @@ -351,10 +350,13 @@ func (s *AgentExecuteStep) Execute(ctx context.Context, pc *module.PipelineConte } } - // Redact secrets from messages before sending to LLM - if !IsNull(guard) { + // Redact secrets from messages before sending to LLM. The secretService + // composite is always registered for a real app (secretsResolverHook runs + // unconditionally), and this step returns early when s.app==nil, so + // svcs.SecretGuard is non-nil here — no IsNull gate needed (D13). + if svcs.SecretGuard != nil { for i := range messages { - guard.CheckAndRedact(&messages[i]) + svcs.SecretGuard.CheckAndRedact(&messages[i]) } } @@ -578,9 +580,9 @@ func (s *AgentExecuteStep) Execute(ctx context.Context, pc *module.PipelineConte } } - // Redact tool results - if !IsNull(guard) { - resultStr = guard.Redact(resultStr) + // Redact tool results (composite always present — see note above; D13). + if svcs.SecretGuard != nil { + resultStr = svcs.SecretGuard.Redact(resultStr) } messages = append(messages, provider.Message{ @@ -841,7 +843,7 @@ func (s *AgentExecuteStep) handleHumanRequestWait(ctx context.Context, toolResul var msg string if req.RequestType == RequestTypeToken { // Do not leak secret values into the agent transcript/LLM context. - // Reference the secret_name from metadata so the agent can read via SecretGuard. + // Reference the secret_name from metadata so the agent can read it via the secret store. secretRef := "the configured secret store" var meta map[string]any if jsonErr := json.Unmarshal([]byte(req.Metadata), &meta); jsonErr == nil { diff --git a/orchestrator/step_human_request.go b/orchestrator/step_human_request.go index 8a00614..7aa84de 100644 --- a/orchestrator/step_human_request.go +++ b/orchestrator/step_human_request.go @@ -70,7 +70,7 @@ func (s *HumanRequestResolveStep) Execute(ctx context.Context, pc *module.Pipeli } // If the request was for a token and metadata specifies a secret_name, - // auto-store the value in the SecretGuard. + // auto-store the value in the secretService composite's provider. s.autoStoreSecret(ctx, svcs.SecretGuard, hrm, requestID, responseData) return &module.StepResult{ @@ -107,9 +107,13 @@ func (s *HumanRequestResolveStep) Execute(ctx context.Context, pc *module.Pipeli } // autoStoreSecret checks if a resolved token request has a secret_name in its metadata, -// and if so, stores the provided value in the SecretGuard. -func (s *HumanRequestResolveStep) autoStoreSecret(ctx context.Context, guard SecretGuardService, hrm HumanRequestService, requestID, responseData string) { - if responseData == "" { +// and if so, stores the provided value via the secretService composite's Provider +// and arms the Redactor so the just-stored token is redacted on the next pass (D3). +// +// The composite is TRULY-OPTIONAL: absence is a nil pointer (D5/D13 — concrete, +// not a Null default), so we nil-check Provider() rather than IsNull. +func (s *HumanRequestResolveStep) autoStoreSecret(ctx context.Context, guard *secretService, hrm HumanRequestService, requestID, responseData string) { + if guard == nil || responseData == "" { return } @@ -139,17 +143,19 @@ func (s *HumanRequestResolveStep) autoStoreSecret(ctx context.Context, guard Sec return } - // SecretGuard is TRULY-OPTIONAL: skip auto-store when absent. - if IsNull(guard) { + // secretService is TRULY-OPTIONAL: skip auto-store when its provider is + // unresolved (nil). D5 nil-check on the accessor path. + if sp := guard.Provider(); sp == nil { return } - if sp := guard.Provider(); sp != nil { - if err := sp.Set(ctx, secretName, value); err != nil { - fmt.Printf("ratchetplugin: failed to store secret %q: %v\n", secretName, err) - return - } - guard.AddKnownSecret(secretName, value) + if err := guard.Provider().Set(ctx, secretName, value); err != nil { + fmt.Printf("ratchetplugin: failed to store secret %q: %v\n", secretName, err) + return } + // D3: arm the Redactor with the just-stored token so the next Redact masks + // it. Dropping this call would silently leak the token in the next + // redaction pass (the failure class the redactor exists to prevent). + guard.Redactor().AddValue(secretName, value) } // newHumanRequestResolveFactory returns a plugin.StepFactory for "step.human_request_resolve". diff --git a/orchestrator/step_webhook.go b/orchestrator/step_webhook.go index f562b88..2f42d7f 100644 --- a/orchestrator/step_webhook.go +++ b/orchestrator/step_webhook.go @@ -128,10 +128,13 @@ func (s *WebhookProcessStep) Execute(ctx context.Context, pc *module.PipelineCon }, nil } -// resolveSecretValue retrieves a secret value from the SecretGuardService. -// Returns "" when the guard is absent (Null) or the secret is not found. -func resolveSecretValue(ctx context.Context, guard SecretGuardService, secretName string) string { - if IsNull(guard) { +// resolveSecretValue retrieves a secret value from the secretService composite. +// Returns "" when the composite is absent (nil) or its provider is unresolved +// (nil) or the secret is not found. Absence is a nil pointer (D5/D13 — the +// composite is concrete, not a Null default), so we nil-check the pointer and +// Provider() rather than IsNull. +func resolveSecretValue(ctx context.Context, guard *secretService, secretName string) string { + if guard == nil { return "" } sp := guard.Provider() diff --git a/orchestrator/test_provider_helpers_test.go b/orchestrator/test_provider_helpers_test.go index 27f106b..cf84ec8 100644 --- a/orchestrator/test_provider_helpers_test.go +++ b/orchestrator/test_provider_helpers_test.go @@ -94,7 +94,7 @@ func SetupE2EAgent(t *testing.T, p provider.Provider) *E2ETestEnv { } // Set up components - sg := NewSecretGuard(&mockSecretsProvider{secrets: map[string]string{}}, "test") + sg := newTestSecretService(&mockSecretsProvider{secrets: map[string]string{}}) rec := NewTranscriptRecorder(db, sg) tr := NewToolRegistry() diff --git a/orchestrator/transcript.go b/orchestrator/transcript.go index 8aa889a..d352f71 100644 --- a/orchestrator/transcript.go +++ b/orchestrator/transcript.go @@ -25,21 +25,21 @@ type TranscriptEntry struct { // TranscriptRecorder records agent interactions to the database. type TranscriptRecorder struct { - db *sql.DB - guard *SecretGuard + db *sql.DB + svc *secretService } -func NewTranscriptRecorder(db *sql.DB, guard *SecretGuard) *TranscriptRecorder { - return &TranscriptRecorder{db: db, guard: guard} +func NewTranscriptRecorder(db *sql.DB, svc *secretService) *TranscriptRecorder { + return &TranscriptRecorder{db: db, svc: svc} } // Record saves a transcript entry to the database. func (tr *TranscriptRecorder) Record(ctx context.Context, entry TranscriptEntry) error { redacted := 0 content := entry.Content - if tr.guard != nil { + if tr.svc != nil { original := content - content = tr.guard.Redact(content) + content = tr.svc.Redact(content) if content != original { redacted = 1 } diff --git a/orchestrator/webhook.go b/orchestrator/webhook.go index 31b1543..c047ce3 100644 --- a/orchestrator/webhook.go +++ b/orchestrator/webhook.go @@ -30,13 +30,12 @@ type Webhook struct { // WebhookManager manages webhook configurations and processing logic. type WebhookManager struct { - db *sql.DB - guard *SecretGuard + db *sql.DB } // NewWebhookManager creates a new WebhookManager. -func NewWebhookManager(db *sql.DB, guard *SecretGuard) *WebhookManager { - return &WebhookManager{db: db, guard: guard} +func NewWebhookManager(db *sql.DB) *WebhookManager { + return &WebhookManager{db: db} } // Create inserts a new webhook into the database. diff --git a/orchestrator/webhook_test.go b/orchestrator/webhook_test.go index 445b79c..571d3ed 100644 --- a/orchestrator/webhook_test.go +++ b/orchestrator/webhook_test.go @@ -47,7 +47,7 @@ func TestWebhookManagerCreate(t *testing.T) { db := setupWebhookTestDB(t) defer func() { _ = db.Close() }() - wm := NewWebhookManager(db, nil) + wm := NewWebhookManager(db) ctx := context.Background() wh := Webhook{ @@ -88,7 +88,7 @@ func TestWebhookManagerDelete(t *testing.T) { db := setupWebhookTestDB(t) defer func() { _ = db.Close() }() - wm := NewWebhookManager(db, nil) + wm := NewWebhookManager(db) ctx := context.Background() wh := Webhook{ID: "test-id", Source: "generic", Name: "del-test", Enabled: true} @@ -113,7 +113,7 @@ func TestWebhookManagerGetBySource(t *testing.T) { db := setupWebhookTestDB(t) defer func() { _ = db.Close() }() - wm := NewWebhookManager(db, nil) + wm := NewWebhookManager(db) ctx := context.Background() hooks := []Webhook{ @@ -147,7 +147,7 @@ func TestWebhookManagerGetBySource(t *testing.T) { } func TestWebhookVerifySignatureGitHub(t *testing.T) { - wm := NewWebhookManager(nil, nil) + wm := NewWebhookManager(nil) secret := "my-github-secret" payload := []byte(`{"action":"opened","issue":{"title":"test"}}`) @@ -173,7 +173,7 @@ func TestWebhookVerifySignatureGitHub(t *testing.T) { } func TestWebhookVerifySignatureGeneric(t *testing.T) { - wm := NewWebhookManager(nil, nil) + wm := NewWebhookManager(nil) secret := "my-webhook-secret" payload := []byte(`{"type":"deploy","env":"production"}`) @@ -188,7 +188,7 @@ func TestWebhookVerifySignatureGeneric(t *testing.T) { } func TestWebhookVerifySignatureSlack(t *testing.T) { - wm := NewWebhookManager(nil, nil) + wm := NewWebhookManager(nil) secret := "slack-signing-secret" payload := []byte(`{"type":"event_callback","event":{"type":"app_mention"}}`) @@ -205,7 +205,7 @@ func TestWebhookVerifySignatureSlack(t *testing.T) { } func TestWebhookVerifySignatureNoSecret(t *testing.T) { - wm := NewWebhookManager(nil, nil) + wm := NewWebhookManager(nil) payload := []byte(`{"event":"push"}`) // Empty secret should return true (no verification required) @@ -220,7 +220,7 @@ func TestWebhookVerifySignatureNoSecret(t *testing.T) { } func TestWebhookMatchesFilter(t *testing.T) { - wm := NewWebhookManager(nil, nil) + wm := NewWebhookManager(nil) tests := []struct { source string @@ -265,7 +265,7 @@ func TestWebhookMatchesFilter(t *testing.T) { } func TestWebhookExtractEventType(t *testing.T) { - wm := NewWebhookManager(nil, nil) + wm := NewWebhookManager(nil) tests := []struct { name string @@ -331,7 +331,7 @@ func TestWebhookExtractEventType(t *testing.T) { } func TestWebhookRenderTaskTemplate(t *testing.T) { - wm := NewWebhookManager(nil, nil) + wm := NewWebhookManager(nil) tests := []struct { name string @@ -428,7 +428,7 @@ func TestWebhookManagerListEmpty(t *testing.T) { db := setupWebhookTestDB(t) defer func() { _ = db.Close() }() - wm := NewWebhookManager(db, nil) + wm := NewWebhookManager(db) ctx := context.Background() hooks, err := wm.List(ctx) @@ -444,7 +444,7 @@ func TestWebhookIDAutoGenerated(t *testing.T) { db := setupWebhookTestDB(t) defer func() { _ = db.Close() }() - wm := NewWebhookManager(db, nil) + wm := NewWebhookManager(db) ctx := context.Background() // No ID provided — should be auto-generated diff --git a/orchestrator/wiring_integration_test.go b/orchestrator/wiring_integration_test.go index c9f4a40..1a47074 100644 --- a/orchestrator/wiring_integration_test.go +++ b/orchestrator/wiring_integration_test.go @@ -44,7 +44,7 @@ func injectAll(t *testing.T, app *mockApp) *sql.DB { t.Helper() db := openTestDB(t) - sg := NewSecretGuard(&mockSecretsProvider{secrets: map[string]string{}}, "test") + sg := newTestSecretService(&mockSecretsProvider{secrets: map[string]string{}}) tr := NewToolRegistry() bb := NewBlackboard(db, nil) if err := bb.Migrate(context.Background()); err != nil { @@ -64,7 +64,7 @@ func injectAll(t *testing.T, app *mockApp) *sql.DB { app.services["ratchet-sub-agent-manager"] = NewSubAgentManager(db, 0, 0) app.services["ratchet-skill-manager"] = NewSkillManager(db, "") app.services["ratchet-container-manager"] = NewContainerManager(db) - app.services["ratchet-webhook-manager"] = NewWebhookManager(db, sg) + app.services["ratchet-webhook-manager"] = NewWebhookManager(db) app.services["ratchet-memory-store"] = ms app.services["ratchet-transcript-recorder"] = rec @@ -95,7 +95,6 @@ func TestResolveServices_AllInjectedReturnsRealInterfaces(t *testing.T) { }{ {"Blackboard", b.Blackboard}, {"ToolRegistry", b.ToolRegistry}, - {"SecretGuard", b.SecretGuard}, {"Approval", b.Approval}, {"HumanRequest", b.HumanRequest}, {"SubAgent", b.SubAgent}, @@ -110,6 +109,11 @@ func TestResolveServices_AllInjectedReturnsRealInterfaces(t *testing.T) { t.Errorf("resolveServices: %s is Null after injection, want real", c.name) } } + // SecretGuard is a concrete *secretService pointer (not a Null default), so + // IsNull can't detect it — assert non-nil directly (D14/D13). + if b.SecretGuard == nil { + t.Errorf("resolveServices: SecretGuard is nil after injection, want real") + } // Concrete types surface through the interface where no adapter is used. if _, ok := b.Blackboard.(*Blackboard); !ok { @@ -146,7 +150,6 @@ func TestResolveServices_EmptyAppReturnsAllNulls(t *testing.T) { }{ {"Blackboard", b.Blackboard}, {"ToolRegistry", b.ToolRegistry}, - {"SecretGuard", b.SecretGuard}, {"Approval", b.Approval}, {"HumanRequest", b.HumanRequest}, {"SubAgent", b.SubAgent}, @@ -161,6 +164,10 @@ func TestResolveServices_EmptyAppReturnsAllNulls(t *testing.T) { t.Errorf("resolveServices(empty): %s = %T, want Null default", c.name, c.svc) } } + // SecretGuard is a concrete *secretService pointer; absence is nil (D13/D14). + if b.SecretGuard != nil { + t.Errorf("resolveServices(empty): SecretGuard = %T, want nil", b.SecretGuard) + } if b.DB != nil { t.Errorf("resolveServices(empty): DB = %T, want nil", b.DB) } @@ -507,8 +514,8 @@ func TestResolveServices_RefactoredWebhookProcessMultiDep(t *testing.T) { if b.DB == nil { t.Fatal("precondition: DB is nil after wiring ratchet-db") } - if IsNull(b.SecretGuard) { - t.Fatal("precondition: SecretGuard is Null after injection") + if b.SecretGuard == nil { + t.Fatal("precondition: SecretGuard is nil after injection") } step := &WebhookProcessStep{name: "wh-process-test", app: app} @@ -597,7 +604,6 @@ func TestResolveServices_AgentExecuteBundleResolution(t *testing.T) { eb := resolveServices(empty) for name, svc := range map[string]any{ "ToolRegistry": eb.ToolRegistry, - "SecretGuard": eb.SecretGuard, "Memory": eb.Memory, "Approval": eb.Approval, "HumanRequest": eb.HumanRequest, @@ -610,4 +616,8 @@ func TestResolveServices_AgentExecuteBundleResolution(t *testing.T) { t.Errorf("agent_execute dep %s = %T on empty app, want Null", name, svc) } } + // SecretGuard is a concrete *secretService pointer; absence is nil (D13/D14). + if eb.SecretGuard != nil { + t.Errorf("agent_execute dep SecretGuard = %T on empty app, want nil", eb.SecretGuard) + } }