Skip to content

refactor(environment): extract shared helpers for command-based providers#1730

Merged
rumpl merged 1 commit intodocker:mainfrom
dgageot:simplify-env-2
Feb 16, 2026
Merged

refactor(environment): extract shared helpers for command-based providers#1730
rumpl merged 1 commit intodocker:mainfrom
dgageot:simplify-env-2

Conversation

@dgageot
Copy link
Member

@dgageot dgageot commented Feb 14, 2026

Deduplicate command execution and binary lookup logic from pass, keychain, and credential helper providers into shared runCommand and lookupBinary helpers in a new cmd_provider.go file.

Assisted-By: cagent

…ders

Deduplicate command execution and binary lookup logic from pass,
keychain, and credential helper providers into shared runCommand
and lookupBinary helpers in a new cmd_provider.go file.

Assisted-By: cagent
@dgageot dgageot requested a review from a team as a code owner February 14, 2026 09:18
Copy link

@docker-agent docker-agent bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Found 1 issue in the refactoring: The new lookupBinary helper function masks non-ErrNotFound errors, which could make debugging difficult when system issues (like permission errors) occur.

The rest of the refactoring looks solid - it successfully deduplicates command execution and binary lookup logic without changing the functional behavior of the credential providers.

slog.Warn("failed to lookup `"+name+"` binary", "error", err)
}
if path == "" {
return notFoundErr
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error masking bug: Non-ErrNotFound errors are logged but not returned

When exec.LookPath fails with an error other than ErrNotFound (e.g., permission denied, I/O errors), the function logs a warning at line 34 but then returns the generic notFoundErr parameter here instead of the actual error.

This means callers will receive KeychainNotAvailableError or PassNotAvailableError when the real issue might be a permission problem or system configuration issue, losing critical debugging information.

Suggested fix:

func lookupBinary(name string, notFoundErr error) error {
	path, err := exec.LookPath(name)
	if err != nil {
		if errors.Is(err, exec.ErrNotFound) {
			return notFoundErr
		}
		// Return the actual error for non-NotFound cases
		slog.Warn("failed to lookup `"+name+"` binary", "error", err)
		return fmt.Errorf("failed to lookup %s: %w", name, err)
	}
	return nil
}

Or at minimum, check the error first rather than relying on the empty path check.

@rumpl rumpl merged commit 7445ef8 into docker:main Feb 16, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants