fix(#1661): expand env vars for non-Unix shells (PowerShell/cmd)#1666
fix(#1661): expand env vars for non-Unix shells (PowerShell/cmd)#1666
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRefactors variable substitution logic across multiple evaluation modules from regex-based callbacks to manual index-based parsing, introduces a single-quoted variable detection helper, and centralizes shell-aware evaluation options for improved cross-platform command interpolation support. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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. Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cmn/eval/resolve.go (1)
114-124:⚠️ Potential issue | 🟡 MinorRemove
extractVarKeyor move it to a test helper file.The function is not called by
replaceVars(which extracts keys directly from submatch indices) or any production code. It exists only inresolve_test.go, making it dead code in production. Either remove it or relocate it to a test utilities file if it serves as a reusable test helper.
🤖 Fix all issues with AI agents
In `@internal/runtime/builtin/command/command.go`:
- Around line 362-377: The commandEvalOptions logic is correct but lacks an
explicit comment explaining the intentional fish exception; update the comment
above commandEvalOptions (and/or near IsUnixLikeShell/IsNixShell references) to
state that fish intentionally returns false from IsUnixLikeShell so Dagu
performs ${VAR} expansion for fish (due to -e flag incompatibility), and mention
that PowerShell/cmd are handled by leaving ExpandEnv=true; leave the
implementation of commandEvalOptions, eval.WithoutDollarEscape(),
eval.WithoutExpandEnv(), cmdutil.IsUnixLikeShell, and cmdutil.IsNixShell
unchanged.
🧹 Nitpick comments (3)
internal/cmn/eval/resolve.go (1)
126-130:isSingleQuotedVaruses a heuristic that may misidentify nested quote contexts.The check only looks at the immediately adjacent characters (
input[start-1]andinput[end]), so patterns like'foo'${BAR}'baz'would incorrectly treat${BAR}as single-quoted (the'before it is actually closing a prior quoted segment). This is acceptable for the targeted use cases (nu shell$'...'syntax, simple quoting), but worth documenting.internal/runtime/builtin/command/eval_options_test.go (1)
28-89: Good integration test covering the core shell types.The test validates the full flow through
step.EvalOptions(ctx)→ registeredGetEvalOptions→commandEvalOptions, which is more robust than unit-testing the helper alone.Consider adding a case for an empty/unset shell to verify the fallback path (
len(shell) == 0incommandEvalOptions), and for other shells likebash,zsh, ornufor completeness.internal/cmn/eval/envscope.go (1)
172-177: Add defensive guard to handle potential future regex changes safely.The current regex
\$\{([^}]+)\}|\$([a-zA-Z0-9_][a-zA-Z0-9_]*)guarantees that exactly one of the two capture groups will match, so bothloc[2]andloc[4]cannot be negative simultaneously. However, adding an explicit check before accessingloc[4]provides defensive protection against future regex modifications that might break this invariant.Optional defensive guard
var key string if loc[2] >= 0 { // Group 1: ${...} key = s[loc[2]:loc[3]] - } else { // Group 2: $VAR + } else if loc[4] >= 0 { // Group 2: $VAR key = s[loc[4]:loc[5]] + } else { + // Neither group captured — preserve original text. + b.WriteString(match) + continue }
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1666 +/- ##
==========================================
- Coverage 70.18% 69.84% -0.35%
==========================================
Files 345 345
Lines 38664 38817 +153
==========================================
- Hits 27135 27110 -25
- Misses 9361 9398 +37
- Partials 2168 2309 +141
... and 25 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
Fixes #1661
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests