-
Notifications
You must be signed in to change notification settings - Fork 151
feat: add environment variable placeholder expansion for agent commands (#604) #607
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add environment variable placeholder expansion for agent commands (#604) #607
Conversation
|
I've tested this thoroughly with different placeholder scenarios and it all works well! I have some example YAML files I created during development - would you like me to include those in this PR at like |
|
Thank you! Adding examples would be great yes. And if you have the time, a couple of lines of documentation in docs/usage.md would be amazing. |
cmd/root/run.go
Outdated
| if msg, ok := cmds[trimmed]; ok { | ||
| commandFirstMessage = &msg | ||
| // Expand $placeholders with environment variable values | ||
| env := environment.NewDefaultProvider() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting from my phone, I think this code should be in the teamloader, that way all the other commands (exec, api…) can have the same behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! That makes sense, I'll take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick question about the current behavior right now, undefined variables cause an error. This means if a user runs -c <some_command> without defining a variable used in that command, it fails. However, if I move the expansion logic to the teamloader, it attempts to expand all variables during agent loading and fails if any variable is undefined.
I’m considering introducing a more lenient expand function that still allows the agent to load even if some variables are undefined in that case, those variables would just be replaced with empty strings instead of causing an error. Does that sound right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to not fail but replace with empty, after all echo $SOME_VAR_THAT_DOESNT_EXIST doesn't fail but just returns an empty string.
In the future we might add the same interpolation logic as compose has but we can do that in a followup :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after all
echo $SOME_VAR_THAT_DOESNT_EXISTdoesn't fail but just returns an empty string.
Yeah, that’s what I was thinking too, makes perfect sense. I’ll get it done then, glad I asked! Thanks :)
Absolutely! I’ll get it done |
pkg/teamloader/teamloader.go
Outdated
| agentsByName := make(map[string]*agent.Agent) | ||
|
|
||
| for name, agentConfig := range cfg.Agents { | ||
| // Expand environment variable placeholders in commands |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the expansion happen just when the command is about to run?
If that's too complicated, that's ok. I just want things to be as lazy as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @dgageot! I appreciate the feedback about lazy evaluation.
Quick question: if we do lazy expansion, doesn't that essentially mean we're back to my original implementation - expanding in run.go when the command is used?
The only difference would be calling a teamloader helper function instead of environment.Expand() directly. @rumpl suggested keeping expansion logic in teamloader for architectural consistency, but for lazy expansion, the actual call would happen in run.go.
So is this the right direction - keep the expansion utility in teamloader but call it lazily from run.go when the command is actually used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll sync with @rumpl and will come back to you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small update, we didn't have the time to sync today sorry, we'll get to it as soon as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, No problem
Implement support for $placeholder syntax in commands defined in agent.yaml
files. When a command is invoked using the -c/--command flag, any
$VARIABLE_NAME or ${VARIABLE_NAME} placeholders in the command string are
now automatically replaced with their corresponding environment variable
values.
The expansion uses the existing
environment.Expand() function and environment.NewDefaultProvider() to access
environment variables from multiple sources (OS env, secrets, keychain, etc.).
Implementation details:
- Added placeholder expansion in cmd/root/run.go after command lookup
- Uses environment.Expand() for consistent variable resolution
- Returns clear error messages when required environment variables are missing
- Maintains backward compatibility with commands that don't use placeholders
Fixes docker#604
Signed-off-by: Sadique Azmi <[email protected]>
Signed-off-by: David Gageot <[email protected]>
5c5b510 to
0bdcf45
Compare
|
Thanks! |
Description
Implements support for
$placeholdersyntax in agent commands defined inagent.yamlfiles. When a command is invoked using the-c/--commandflag, any$VARIABLE_NAMEor${VARIABLE_NAME}placeholders are automatically replaced with their corresponding environment variable values.Fixes #604
Changes
cmd/root/run.go:pkg/environmentpackageenvironment.NewDefaultProvider()to access env vars from multiple sources (OS env, secrets, keychain, etc.)Implementation Details
The expansion leverages existing infrastructure:
environment.Expand()- Handles both$VARand${VAR}syntaxenvironment.NewDefaultProvider()- Provides access to multiple environment sourcesMaintains full backward compatibility - commands without placeholders work exactly as before.
Example Usage
Before:
$ cagent run agent.yaml -c greet # Agent receives literal: "Say hello to $USER"After:
Agent configuration example:
Testing
Tested with Docker Model Runner (DMR) using
ai/gpt-oss:latestManual Testing
Tested with multiple scenarios:
$USER)$PROJECT_NAME,$OWNER,$LOCATION)$VARand${VAR}syntaxChecklist
task build)task lint)environmentpackage infrastructureSigned-off-by: Sadique Azmi [email protected]