engine: secretsCommand via HostExecutor (closes #23)

[bilby91]

Summary

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. Reuses the HostExecutor interface from #33 — no new caller wiring.

Closes #23.

Surface

• config.ResolvedConfig.SecretsCommand — single LifecycleCommand (not a slice: per spec, secretsCommand is not contributed by feature/base-image metadata layers).
• UpOptions.RunSecretsCommand — opt-in, mirrors RunInitializeCommand. Default false because arbitrary host execution is security-sensitive.
• No changes to the HostExecutor interface.

Behavior

Wiring (up.go):

• createFresh / createFreshCompose invoke collectSecretsEnv between reconcileRemoteUserUID and buildRunSpec/compose-override. Output is merged with opts.ExtraContainerEnv; caller-supplied env wins on key collision (explicit intent trumps spec hooks).
• attachExisting deliberately 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 pass Recreate=true.

Parser (parseSecretsKV) — intentionally narrow:

• split on the first =, trim key whitespace, take value verbatim
• skip blank lines, # comments (after leading whitespace), lines without =, and lines with empty keys
• handle CRLF
• no quote stripping or escape interpretation: secretsCommand stdout is program-generated; silently mangling literal \" or \\ would be worse than rejecting awkward inputs

Failure modes (mirror initializeCommand):

• Missing executor → *LifecycleError wrapping ErrHostExecutorNotConfigured
• Non-zero host exit → *LifecycleError with exit code, stdout, stderr
• Phase tag is the synthetic \"secretsCommand\" so callers distinguish it from initializeCommand failures via errors.As + Phase

Test plan

• go test ./... — all green
• go test -race ./... — clean
• go vet ./... (incl. -tags=integration)
• gofmt clean

New 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 in RunSpec.Env alongside containerEnv.
• TestSecretsCommand_ExtraContainerEnvOverridesSecrets — collision precedence.
• TestSecretsCommand_SkippedByDefault — confirms RunSecretsCommand=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

  • Support for a new secretsCommand in devcontainer.json to run host-side before container start
  • Command output parsed as key=value and injected into container environment (caller env overrides)
  • New option to enable secretsCommand execution; only runs on fresh container creation and requires a host executor

• Tests

  • Added coverage for parsing, execution behavior, merging, skipping, and error handling

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: be54e783-0d6a-41f4-9da7-20b3a9d1b0ef

📥 Commits

Reviewing files that changed from the base of the PR and between 09e5a7b and b26dc54.

📒 Files selected for processing (1)

• up.go

🚧 Files skipped from review as they are similar to previous changes (1)

• up.go

---

📝 Walkthrough

Walkthrough

This PR implements the secretsCommand lifecycle hook: adds schema fields, decodes and substitutes the command, runs optional host-side secrets commands, parses stdout as key=value lines, merges results into the container environment (caller-provided env wins), and adds tests and an opt-in UpOptions flag.

SecretsCommand Lifecycle Feature

Layer / File(s)	Summary
Config Schema config/raw.go, config/resolved.go	SecretsCommand field added to rawConfig and ResolvedConfig to carry the host-side command through resolution.
Config Resolution & Substitution config/resolve.go	resolveFromRaw decodes /secretsCommand with scoped error messages; substituteAll runs substituteCommand on the decoded command.
Host-Side Execution secrets.go	runSecretsCommand orchestrates single or parallel named commands; execSecretsHost invokes HostExecutor and wraps failures in a LifecycleError with phase phaseSecretsCommand; parseSecretsKV parses newline-delimited KEY=VALUE into a map.
Container Integration up.go	UpOptions.RunSecretsCommand added; collectSecretsEnv conditionally runs secretsCommand; createFresh and createFreshCompose merge collected secrets into the container run spec / compose override; compose reattach path disables execution.
Tests & Validation secrets_test.go	Tests for parsing edge cases, HostExecutor requirement and error propagation, environment merging and precedence, optional execution, and non-zero exit handling.

Sequence Diagram(s)

sequenceDiagram
  participant Caller as Engine.Up
  participant Resolver as config.resolveFromRaw
  participant Executor as HostExecutor
  participant Parser as parseSecretsKV
  participant Runtime as buildRunSpec/Container
  Caller->>Resolver: read ResolvedConfig.SecretsCommand
  Caller->>Executor: ExecHost(secretsCommand)
  Executor-->>Caller: stdout/stderr/exitCode
  Caller->>Parser: parse stdout -> map
  Caller->>Runtime: merge parsed env (caller overrides) -> create container

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• crunchloop/devcontainer#33: Introduces HostExecutor infrastructure used by this PR to run host-side lifecycle commands.

Poem

🐰 I hopped to run commands on the host,

lines of secrets I parse like toast,
Keys trimmed neat, values left true,
Merged gently where callers rule,
A tiny rabbit brings envs to you.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: adding secretsCommand support via HostExecutor with a reference to the related issue.
Linked Issues check	✅ Passed	The PR fully implements all coding objectives from issue #23: secretsCommand is added to config, executed via HostExecutor, stdout is parsed and merged into container environment, and the feature is opt-in via RunSecretsCommand.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to implementing secretsCommand support as specified in issue #23; no unrelated modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch secrets-command

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 7222bb3 and 09e5a7b.

📒 Files selected for processing (6)

• config/raw.go
• config/resolve.go
• config/resolved.go
• secrets.go
• secrets_test.go
• up.go