feat(module): add step.secret_set step#405
Merged
Merged
Conversation
TDD first commit: tests reference NewSecretSetStepFactory and SecretSetProvider which do not exist yet. Covers happy-path single/multiple key writes, template resolution from pipeline context, provider errors, missing module, wrong service type, and nil app. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds step.secret_set, the write-side mirror of step.secret_fetch.
Resolves Go template values against the pipeline context and writes
each key to the named secrets module via provider.Set. Returns
{"set_keys": [...]} for observability. Registers in plugin manifest,
StepFactories, KnownStepTypes, schema registries, golden file, and
DOCUMENTATION.md.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new pipeline step type, step.secret_set, intended to persist template-resolved values into a configured secrets provider/module (as a write-side mirror of step.secret_fetch).
Changes:
- Introduces
SecretSetStepimplementation + unit tests for config validation, template resolution, and provider write behavior. - Registers the new step in the pipelinesteps plugin factory/type lists and wfctl type registry.
- Updates schemas (step/module + editor golden) and documentation to include
step.secret_set.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| schema/testdata/editor-schemas.golden.json | Adds editor schema entry for step.secret_set config fields. |
| schema/step_schema_builtins.go | Registers step schema for step.secret_set, including set_keys output. |
| schema/schema.go | Adds step.secret_set to known core module/step types. |
| schema/module_schema.go | Adds module schema entry for step.secret_set (UI/config introspection). |
| plugins/pipelinesteps/plugin_test.go | Extends factory coverage list to include step.secret_set. |
| plugins/pipelinesteps/plugin.go | Registers step.secret_set in plugin metadata and factory map. |
| module/pipeline_step_secret_set.go | Implements the new SecretSetStep and factory. |
| module/pipeline_step_secret_set_test.go | Adds unit tests covering factory validation + execution behavior. |
| cmd/wfctl/type_registry.go | Adds step.secret_set to CLI-known step types. |
| DOCUMENTATION.md | Documents step.secret_set and adds it to the step catalog table. |
Comment on lines
+88
to
+104
| setKeys := make([]string, 0, len(s.secrets)) | ||
|
|
||
| for secretKey, valueTemplate := range s.secrets { | ||
| // Resolve the value template against the current pipeline context. | ||
| // This enables dynamic values such as form fields from prior steps: | ||
| // "{{.steps.form.client_id}}" | ||
| resolvedValue, resolveErr := s.tmpl.Resolve(valueTemplate, pc) | ||
| if resolveErr != nil { | ||
| return nil, fmt.Errorf("secret_set step %q: failed to resolve value for %q: %w", s.name, secretKey, resolveErr) | ||
| } | ||
|
|
||
| if setErr := provider.Set(ctx, secretKey, resolvedValue); setErr != nil { | ||
| return nil, fmt.Errorf("secret_set step %q: failed to set secret %q: %w", s.name, secretKey, setErr) | ||
| } | ||
|
|
||
| setKeys = append(setKeys, secretKey) | ||
| } |
There was a problem hiding this comment.
Secrets are written by iterating over a map, so the write order is non-deterministic. If provider.Set fails partway through, which keys were written before the failure will vary run-to-run. Consider sorting the keys first and writing in a deterministic order (you can still return a sorted set_keys output).
Comment on lines
+51
to
+56
| // mockAppWithSetProvider registers a secrets.Provider that supports Set into a MockApplication. | ||
| func mockAppWithSetProvider(name string, p SecretSetProvider) *MockApplication { | ||
| app := NewMockApplication() | ||
| app.Services[name] = p | ||
| return app | ||
| } |
There was a problem hiding this comment.
Tests cover the happy path with a mock SecretSetProvider, but they don't exercise the documented/common case of using the built-in secrets modules (secrets.aws / secrets.vault). Adding a test that registers a real SecretsAWSModule/SecretsVaultModule (or a minimal stub matching how those modules are registered) would catch the current interface mismatch and prevent regressions.
Comment on lines
+116
to
+124
| func (s *SecretSetStep) resolveProvider() (SecretSetProvider, error) { | ||
| svc, ok := s.app.SvcRegistry()[s.moduleName] | ||
| if !ok { | ||
| return nil, fmt.Errorf("secret_set step %q: secrets module %q not found in service registry", s.name, s.moduleName) | ||
| } | ||
| provider, ok := svc.(SecretSetProvider) | ||
| if !ok { | ||
| return nil, fmt.Errorf("secret_set step %q: service %q does not implement SecretSetProvider (Set method)", s.name, s.moduleName) | ||
| } |
There was a problem hiding this comment.
resolveProvider requires the service itself to implement SecretSetProvider (Set). The built-in secrets modules (e.g., module/SecretsAWSModule and module/SecretsVaultModule) register themselves as the service instance and currently don't implement Set, so step.secret_set will error with “does not implement SecretSetProvider” when used with those modules. Fix by either adding Set methods on those modules that delegate to their underlying provider, or by updating resolveProvider to detect modules exposing Provider() and use that provider's Set implementation.
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
- Document partial-failure behavior in Execute godoc (earlier writes committed, no rollback — secrets backends lack transactions) - Document that empty resolved values are permitted (clearing a secret) - Add TestSecretSetStep_WhitespaceOnlyKey (factory validation) - Add TestSecretSetStep_PartialFailure (multi-key with mid-write error) - Clarify mock provider comment re: broader interface for test verification Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… write order Address Copilot review feedback on PR #405: - resolveProvider now falls through to Provider() accessor when the service doesn't implement SecretSetProvider directly. This matches how SecretsAWSModule/SecretsVaultModule/SecretsKeychainModule work: they expose Get on the wrapper but Set only on the underlying Provider(). - Sort keys before iterating for writes, so partial-failure behavior is deterministic regardless of map iteration order. - Add TestSecretSetStep_ProviderAccessorFallback exercising the indirect resolution path. - Fix misleading comment claiming modules implement Set directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove premature SecretsKeychainModule references from comments (secrets.keychain is in a separate PR) - Remove redundant sort.Strings(setKeys) — keys already built in sorted order from sortedKeys iteration - Add specific error when Provider() accessor returns nil, distinguishing uninitialized module from wrong service type Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CodeQL flagged the variable `secretKey` as sensitive data flowing to log calls. The variable holds the key *name* (e.g. "client_id"), not the secret value. Renaming to `keyName` avoids the heuristic match while preserving clarity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines
+109
to
+120
| valueTemplate := s.secrets[keyName] | ||
| // Resolve the value template against the current pipeline context. | ||
| // This enables dynamic values such as form fields from prior steps: | ||
| // "{{.steps.form.client_id}}" | ||
| resolvedValue, resolveErr := s.tmpl.Resolve(valueTemplate, pc) | ||
| if resolveErr != nil { | ||
| return nil, fmt.Errorf("secret_set step %q: failed to resolve value for %q: %w", s.name, keyName, resolveErr) | ||
| } | ||
|
|
||
| if setErr := provider.Set(ctx, keyName, resolvedValue); setErr != nil { | ||
| return nil, fmt.Errorf("secret_set step %q: failed to set secret %q: %w", s.name, keyName, setErr) | ||
| } |
There was a problem hiding this comment.
In non-strict template mode, a missing map key resolves to the literal string "" (see pipeline.TemplateEngine behavior). Because this step permits empty values and does not validate the resolved string, a typo in a template can silently write "" (or a string containing it) into the secrets backend. Consider failing fast when the resolved value is "" (or contains it) for templated inputs, or resolving with a strict/"missingkey=error" pass before writing to prevent accidental secret corruption.
In non-strict template mode, a missing map key resolves to the literal string "<no value>" rather than returning an error. For display steps this is acceptable, but for secret_set it would silently corrupt the secrets backend. Fail fast when the resolved value contains the sentinel, directing users to check for typos in template keys. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
step.secret_set, the write-side mirror ofstep.secret_fetch. Writes template-resolved values to a namedsecrets.Providermodule.Motivation
Pipeline-driven setup flows (e.g., OAuth credential storage in zoom-mcp) need to persist user-supplied values to a secrets provider without leaving the YAML pipeline.
step.secret_setcompletes the read/write pair.Surface
Notes
step.secret_fetchin config loading, module resolution, and error handling.{"set_keys": [...]}in step output for observability.Test plan
go test ./...passes🤖 Generated with Claude Code