{{ message }}
engine: secretsCommand via HostExecutor (closes #23)#35
Merged
Conversation
Implements the spec's secretsCommand: a host-side hook that runs before container start, whose stdout (parsed as KEY=VALUE lines) is merged into the container's environment. Surface: - config.ResolvedConfig.SecretsCommand (single LifecycleCommand — unlike the lifecycle phases, secretsCommand is not contributed by feature/base-image metadata layers per spec). - UpOptions.RunSecretsCommand (opt-in, mirrors RunInitializeCommand for the same reason: arbitrary host execution). - Reuses the HostExecutor interface introduced for #11 — no new caller-facing wiring. Wiring in up.go: - createFresh / createFreshCompose merge secretsCommand output with opts.ExtraContainerEnv before buildRunSpec / compose overrides. Caller-supplied ExtraContainerEnv wins on key collision (explicit intent trumps spec hooks). - Skipped on attachExisting: existing container env is already baked, so re-running would be a silent no-op. Callers wanting refreshed secrets pass Recreate=true. Parser (parseSecretsKV) is intentionally narrow: split on first '=', trim key whitespace, take value verbatim, skip blanks/`#`-comments and lines without '='. No quote stripping or escape interpretation — secretsCommand stdout is program-generated, and silently mangling literal `"` or `\` would be worse than rejecting awkward inputs. Failure mode mirrors initializeCommand: missing executor returns *LifecycleError(ErrHostExecutorNotConfigured); non-zero host exit returns *LifecycleError with the exit code, stdout, stderr. Phase tag is the synthetic "secretsCommand" so callers can distinguish it from initializeCommand failures via errors.As + Phase. Closes #23. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@up.go`:
- Around line 416-419: The call to collectSecretsEnv is running unconditionally
and causing secretsCommand to run on compose reattach paths; change the logic in
the function that calls createFreshCompose so collectSecretsEnv (and thus
secretsCommand) is only invoked when creating a fresh compose (i.e., when
opts.Recreate or the same condition createFreshCompose uses is true).
Concretely, move the extraEnv, err := e.collectSecretsEnv(ctx, cfg, opts) block
behind the branch that performs createFreshCompose (or guard it with if
opts.Recreate { ... }), preserve the existing error return behavior, and ensure
the rest of the code uses extraEnv only when it was populated. Use the existing
symbols createFreshCompose, collectSecretsEnv, secretsCommand, and opts.Recreate
to locate and implement the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18c59735-6326-4a14-8358-ff93da3c3110
📒 Files selected for processing (6)
config/raw.goconfig/resolve.goconfig/resolved.gosecrets.gosecrets_test.goup.go
createFreshCompose runs for both fresh and existing-container compose paths (compose's `up -d` is idempotent and dispatches internally), which leaked secretsCommand execution onto reattach despite the "fresh-only" contract on UpOptions.RunSecretsCommand. The non-compose path was already correct because attachExisting doesn't call collectSecretsEnv. Suppress RunSecretsCommand for the compose-reattach branch in Up: when an existing container is found and Recreate is false, env is already baked and re-running the host hook would be a silent no-op (at best) or an unnecessary fetch of host-side secrets (at worst). Recreate=true already nils out `existing` upstream, so the explicit opt-in path stays intact. Co-Authored-By: Claude Opus 4.7 (1M context) <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
1 participant
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
Implements the spec's
secretsCommand: a host-side hook that runs before container start, whose stdout (parsed asKEY=VALUElines) is merged into the container's environment. Reuses theHostExecutorinterface from #33 — no new caller wiring.Closes #23.
Surface
config.ResolvedConfig.SecretsCommand— singleLifecycleCommand(not a slice: per spec, secretsCommand is not contributed by feature/base-image metadata layers).UpOptions.RunSecretsCommand— opt-in, mirrorsRunInitializeCommand. Default false because arbitrary host execution is security-sensitive.HostExecutorinterface.Behavior
Wiring (
up.go):createFresh/createFreshComposeinvokecollectSecretsEnvbetweenreconcileRemoteUserUIDandbuildRunSpec/compose-override. Output is merged withopts.ExtraContainerEnv; caller-supplied env wins on key collision (explicit intent trumps spec hooks).attachExistingdeliberately does not run secretsCommand: the existing container's env is already baked, so re-running would be a silent no-op. Callers needing refreshed secrets passRecreate=true.Parser (
parseSecretsKV) — intentionally narrow:=, trim key whitespace, take value verbatim#comments (after leading whitespace), lines without=, and lines with empty keys\"or\\would be worse than rejecting awkward inputsFailure modes (mirror
initializeCommand):*LifecycleErrorwrappingErrHostExecutorNotConfigured*LifecycleErrorwith exit code, stdout, stderrPhasetag is the synthetic\"secretsCommand\"so callers distinguish it frominitializeCommandfailures viaerrors.As+PhaseTest plan
go test ./...— all greengo test -race ./...— cleango vet ./...(incl.-tags=integration)gofmtcleanNew tests:
TestParseSecretsKV— table-driven: blanks, comments, CRLF, value-with-=, empty value, key whitespace, verbatim quoted value, no-=skipped, empty-key skipped.TestSecretsCommand_RequiresHostExecutor—errors.Is(err, ErrHostExecutorNotConfigured)+ Phase tag.TestSecretsCommand_MergesIntoContainerEnv— verifies parsed secrets land inRunSpec.EnvalongsidecontainerEnv.TestSecretsCommand_ExtraContainerEnvOverridesSecrets— collision precedence.TestSecretsCommand_SkippedByDefault— confirmsRunSecretsCommand=false(default) bypasses the executor.TestSecretsCommand_NonZeroExitProducesLifecycleError— exit code surfaces via*LifecycleError.ExitCode.No integration test — host execution is hostile to CI (would need a sandbox), and the contract is fully covered by unit tests via the in-memory fake executor.
Summary by CodeRabbit
New Features
Tests