fix: align command condition expansion with shell commands#2076
fix: align command condition expansion with shell commands#2076
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe pull request fixes variable expansion inconsistencies in preconditions by centralizing command evaluation options logic. It updates OpenAPI schema documentation for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/core/capabilities_test.go (1)
93-99: ⚡ Quick winAdd cleanup for globally registered capabilities in subtests.
Each subtest registers into a global registry without unregistering. Adding
t.Cleanup(...)after each registration keeps tests isolated and prevents cross-test state bleed.Suggested patch
RegisterExecutorCapabilities("command-eval-opts-test", ExecutorCapabilities{ Command: true, GetCommandEvalOptions: func(_ context.Context, _ Step) []eval.Option { return []eval.Option{eval.WithoutExpandShell()} }, }) + t.Cleanup(func() { UnregisterExecutorCapabilities("command-eval-opts-test") }) RegisterExecutorCapabilities("script-eval-opts-test", ExecutorCapabilities{ Command: true, Script: true, GetScriptEvalOptions: func(_ context.Context, _ Step) []eval.Option { return []eval.Option{eval.WithNoExpansion()} }, }) + t.Cleanup(func() { UnregisterExecutorCapabilities("script-eval-opts-test") }) RegisterExecutorCapabilities("config-eval-opts-test", ExecutorCapabilities{ Command: true, }) + t.Cleanup(func() { UnregisterExecutorCapabilities("config-eval-opts-test") }) RegisterExecutorCapabilities("legacy-eval-opts-test", ExecutorCapabilities{ Command: true, Script: true, GetEvalOptions: func(_ context.Context, _ Step) []eval.Option { return []eval.Option{eval.WithoutExpandShell()} }, }) + t.Cleanup(func() { UnregisterExecutorCapabilities("legacy-eval-opts-test") })Also applies to: 107-114, 122-125, 132-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/capabilities_test.go` around lines 93 - 99, Each subtest calls RegisterExecutorCapabilities (e.g., RegisterExecutorCapabilities("command-eval-opts-test", ExecutorCapabilities{...})) but never unregisters, so add t.Cleanup calls immediately after each RegisterExecutorCapabilities call to remove the entry when the subtest ends; specifically, after each RegisterExecutorCapabilities("...") add t.Cleanup(func(){ UnregisterExecutorCapabilities("...") }) (use the same registration key string) so tests don’t leak global state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/core/capabilities_test.go`:
- Around line 93-99: Each subtest calls RegisterExecutorCapabilities (e.g.,
RegisterExecutorCapabilities("command-eval-opts-test",
ExecutorCapabilities{...})) but never unregisters, so add t.Cleanup calls
immediately after each RegisterExecutorCapabilities call to remove the entry
when the subtest ends; specifically, after each
RegisterExecutorCapabilities("...") add t.Cleanup(func(){
UnregisterExecutorCapabilities("...") }) (use the same registration key string)
so tests don’t leak global state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a9ec84f-908c-41b4-bb44-0aa7251ba9f2
📒 Files selected for processing (3)
internal/core/capabilities.gointernal/core/capabilities_test.gointernal/runtime/builtin/command/command.go
…and-expansion # Conflicts: # internal/core/capabilities_test.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
commandexpansion semanticsRoot Cause
Command-form conditions were evaluated with
eval.OnlyReplaceVars(), which deferred scoped variables to the shell. That diverged from shellcommand:steps, where Dagu expands the command text before execution. Values like~/...therefore worked in shell commands but failed in conditions because tilde expansion does not happen after shell parameter expansion.Testing
go test ./internal/runtime -run "TestEvalConditions|TestRunner_DAGPreconditions|TestRunner_RepeatPolicy" -count=1go test ./internal/intg -run "TestPreconditionWithDAGEnvVars|TestDAGLevelPreconditionWithEnvVars|TestPreconditionWithHomeRelativeDAGEnvVar|TestRepeatPolicy" -count=1go test ./internal/runtime/builtin/command -run TestCommandExecutor_GetEvalOptions -count=1Closes #2057
Summary by CodeRabbit
Documentation
expectedparameter is omitted versus specified.Tests