Repository navigation
feat(config): expand ${env.X} in hook and script-shell working_dir/env - #3416
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
|
|
||
| for key, value := range overrides { | ||
| entry := key + "=" + value | ||
| entry := key + "=" + path.ExpandEnvRefs(value) |
There was a problem hiding this comment.
[MEDIUM] hookEnv expands ${env.X} at config-load time — unset-at-startup variables silently become empty strings
hookEnv is called during hook handler construction (i.e. when the config is first loaded), so path.ExpandEnvRefs calls os.Getenv once — at that point — not at each invocation. If a variable referenced as ${env.X} is unset in the process environment when the agent starts (even if it is set later via some other mechanism), the expansion silently produces "" and that empty string is permanently baked into the hook's env slice.
Before this PR the hook's env values were passed through verbatim, so users who wrote ${env.X} got the literal string ${env.X} instead of the value — unhelpful, but at least discoverable. With the new behavior, a mis-timed or missing variable is still silently dropped, but the value is now "" rather than the literal reference, making it harder to debug.
Consider resolving ${env.X} lazily inside commandHandler.Run (and the existing run-time path in builtinHandler.Run) so that the final env reflects the OS environment at the time the hook fires, matching the per-tool-env resolution model in script_shell.go (where ExpandEnvRefs is called inside execute, i.e. per-invocation). At minimum, a debug-level log entry when os.Getenv returns "" for a referenced variable would make the failure observable.
There was a problem hiding this comment.
Good catch on observability, addressed in cb8fb9a: path.ExpandEnvRefs now uses os.LookupEnv and logs at debug level when a ${env.X} reference names an unset variable (set-but-empty stays silent — it resolved as configured). Test added in pkg/path/expand_test.go.
On the timing premise: expansion is already lazy. Hook handlers are built per-firing — Executor.runHook calls the factory (and thus hookEnv) on every event dispatch, not at config load — so ${env.X} reflects the OS environment at fire time, matching the per-call expansion in script_shell.go. Added a doc comment on hookEnv making that explicit.
There was a problem hiding this comment.
You're absolutely right — thank you for the correction! I missed that hookEnv is called inside the factory, which Executor.runHook invokes per-firing (line 432 in executor.go), not at config-load time. The expansion is already lazy, and the observability fix addresses the original concern perfectly.
The new os.LookupEnv logging in path.ExpandEnvRefs and the doc comment clarifying the timing both look good — users now get a debug-level heads-up when ${env.X} references an unset variable, and the fact that expansion happens at fire time (not config-load) is explicit.
Apologies for the misreading of the call chain!
Addresses review feedback on docker#3416: silent empty expansion was indistinguishable from a legitimately empty variable; the debug log makes it observable. Also clarifies that hookEnv expansion is per-invocation, not config-load. Assisted-By: Claude
Hook and script-shell tools forwarded
working_dirandenvvalues verbatim toexec.Cmd, so${env.X}references silently did nothing. This closes the last fields tracked in #2615 as affected by that gap (follow-up to #2851, #2944, #3257).working_dirin both hooks and script tools now goes throughpath.ExpandPath, giving~,$VAR,${VAR}, and${env.VAR}the same treatment every otherworking_dirfield already had. Expansion runs before the absolute-path check so~/...overrides work correctly. Aworking_dirthat expands to an empty string (unset variable) logs a warning instead of silently falling back to the working directory.envvalues in hooks and per-tool script config expand only the${env.X}form via a newpath.ExpandEnvRefshelper resolved from the OS environment; bare$Xand${X}stay literal, so values that legitimately contain$(passwords, templates) are never mangled. That is exactly the syntax the load-time warning introduced in #2851 has been flagging as broken, so no working config regresses.Two pre-existing bugs are fixed alongside the expansion work. Per-tool script-shell
env(ScriptShellToolConfig.Env) was wired into config parsing but silently dropped at runtime; it is now merged into the spawned process environment with precedence OS env < toolset env < per-tool env < args.validateConfignow accepts per-tool env keys in thecmdtypo check, rejects keys declared in bothargsandenv, and returns an actionable error for${env.X}used incmd. A second pre-existing bug: shell and script toolsets appendedos.Environ()after the configured env, so host variables silently overrode YAML-configured values underexec.Cmd's last-wins deduplication. Host env is now prepended so config env wins.The now-obsolete
warnExecField/warnHooksConfigwarning machinery inpkg/config/expansion_warnings.gois removed. Docs cover a new expansion reference matrix indocs/configuration/overview/index.mdand updated hooks docs. Tests are added at every layer:pkg/path,pkg/hooks,pkg/tools/builtin/shell, andpkg/config. No schema fields are added; frozen config versions are untouched.task dev(lint + tests + build) is green.Part of #2615. Fail-fast on
${VAR}in JS fields and rich JS expressions in path fields remain as follow-up work.