feat: add 1Password CLI integration for secret resolution#3036
Merged
Conversation
docker-agent
reviewed
Jun 9, 2026
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The 1Password CLI integration is well-implemented. No bugs were found in the changed code.
What was reviewed:
pkg/environment/onepassword.go—OnePasswordProviderdecorator with lazyopbinary lookup, correct pass-through for non-op://values, and safe error handling (returns unset rather than leaking rawop://references).pkg/environment/cmd_provider.go— hardenedlookupBinarynow correctly rejectsexec.ErrDotand non-absolute paths (CWE-426 prevention). The logic is sound: theif !errors.Is(err, exec.ErrNotFound) && !errors.Is(err, exec.ErrDot)guard only warns on unexpected errors.pkg/environment/default.go— wrapping the entireMultiProviderchain withNewOnePasswordProviderensures every resolved value (regardless of which provider supplied it) getsop://resolution applied uniformly.pkg/environment/onepassword_test.go— tests cover plain pass-through, successful resolution, missing variable, and failed resolution.t.Context()usage is idiomatic.docs/guides/secrets/index.md— documentation accurately reflects the implementation behavior including the failure mode (variable treated as unset).
Security considerations verified:
op://references are never forwarded raw to model providers on resolution failure ✅opbinary is resolved to an absolute path before execution ✅exec.ErrDotis treated as not-found (no CWD-relative binary execution) ✅runCommandusesstrings.TrimSpaceon output, preventing trailing-newline contamination of secrets ✅
gtardif
approved these changes
Jun 9, 2026
|
❌ PR Review Failed — The review agent encountered an error and could not complete the review. View logs. |
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
4 participants
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.
LLM provider tokens and other secrets read through the environment provider chain can now be resolved from 1Password. When a resolved environment value starts with
op://, docker-agent resolves it via the 1Password CLI (op read) before handing it to a model provider or tool.The implementation adds a new
OnePasswordProviderinpkg/environment/onepassword.gothat decorates the existing provider chain. It resolvesop://references and passes all other values through unchanged. The provider is wired into the default environment setup so it applies across OS environment variables, run secrets, credential helpers, Docker Desktop, pass, and keychain. Theopbinary is resolved lazily; if it's unavailable or a resolution fails, docker-agent logs a warning and treats the variable as unset rather than forwarding the rawop://reference.As a hardening measure, the shared
lookupBinaryhelper inpkg/environment/cmd_provider.gonow rejectsexec.ErrDotand non-absolute paths, preventing PATH hijacking (CWE-426). This also strengthens the existing pass, keychain, and credential-helper providers.