chore(deps): mvdan.cc/sh/v3 (patch → v3.13.1) #197
chore(deps): mvdan.cc/sh/v3 (patch → v3.13.1) #197gh-worker-campaigns-3e9aa4[bot] wants to merge 10 commits intomainfrom
Conversation
Auto-rebase completeBranch is up to date with Auto-Rebase · Add |
272844f to
55dfc10
Compare
Co-authored-by: dd-octo-sts-03ec73[bot] <256648721+dd-octo-sts-03ec73[bot]@users.noreply.github.com>
55dfc10 to
1c5380e
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f17690cf66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0c3334880
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21df8d6aa4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
@codex review this PR |
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the mvdan.cc/sh bump and the escaped-left-brace compatibility workaround. Overall assessment: needs fixes.
| # | Priority | File | Finding |
|---|---|---|---|
| 1 | interp/runner_expand.go:406 |
Escaped commas/right braces inside the brace form are treated as syntax, producing output that differs from bash |
Coverage
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
| escaped left brace disables expansion | tests/scenarios/shell/var_expand/quoting/escaped_left_brace.yaml |
interp/runner_expand_test.go |
Covered |
escaped comma/right brace inside \{{...}} forms |
— | — | Missing |
|
Iteration 1 self-review result: P0=0, P1=1, P2=0, P3=0. Summary: found a bash-compatibility regression in the escaped-left-brace workaround when commas or right braces inside the brace candidate are themselves backslash-escaped; requested changes with an inline comment. |
Preserve bash quote state for escaped brace metacharacters before mvdan SplitBraces runs, including commas, right braces, and sequence dots. Add regression coverage for escaped delimiters and sequence cases.
|
@codex review this PR |
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the updated escaped brace metacharacter handling after inspecting mvdan/sh master. Overall assessment: safe to merge.
No P0/P1/P2/P3 findings. The workaround now aligns with mvdan/expand's pipeline (FieldsSeq calls syntax.SplitBraces before wordFields quote removal) by quoting backslash-escaped brace metacharacters before SplitBraces, while preserving valid unescaped nested/sequence brace expansions. Regression coverage was added for escaped commas, right braces, sequence dots, and bash comparison passes locally.
Coverage
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
| escaped left brace disables expansion | tests/scenarios/shell/var_expand/quoting/escaped_left_brace.yaml |
interp/runner_expand_test.go |
Covered |
| escaped delimiter after escaped left brace | tests/scenarios/shell/var_expand/quoting/escaped_left_brace_composite.yaml |
interp/runner_expand_test.go |
Covered |
| valid sequence after escaped left brace | tests/scenarios/shell/var_expand/quoting/escaped_left_brace_composite.yaml |
interp/runner_expand_test.go |
Covered |
| escaped comma/right brace/dot in regular brace expansion | — | interp/runner_expand_test.go |
Covered |
|
Iteration 2 self-review result: P0=0, P1=0, P2=0, P3=0. Summary: reviewed the updated implementation against mvdan/sh master ( |
Use strings.Index, which is already allowed for interp, instead of introducing strings.Contains in the escaped brace preprocessor.
|
@codex review this PR |
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the current diff against main, including the mvdan/sh expansion flow (FieldsSeq → syntax.SplitBraces → wordFields) and the local escaped-brace preprocessing. Overall assessment: safe to merge.
No P0/P1/P2/P3 findings. The latest CI fix avoids adding a new interp allowlist symbol, and the escaped brace metacharacter tests cover the changed paths.
Coverage
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
| escaped left brace disables brace expansion | tests/scenarios/shell/var_expand/quoting/escaped_left_brace.yaml |
interp/runner_expand_test.go |
Covered |
| escaped left brace across nested/empty alternatives and parameters | tests/scenarios/shell/var_expand/quoting/escaped_left_brace_composite.yaml |
interp/runner_expand_test.go |
Covered |
| escaped comma/right brace/dot metacharacters | tests/scenarios/shell/var_expand/quoting/escaped_left_brace_composite.yaml |
interp/runner_expand_test.go |
Covered |
| interp allowed-symbol compliance for string search | — | analysis allowlist tests |
Covered |
|
Iteration 3 self-review result: P0=0, P1=0, P2=0, P3=0. Summary: reviewed current HEAD (including the interp allowed-symbol fix) against mvdan/sh expansion flow and bash-compatibility coverage; no findings. |
|
@codex review this PR |
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
Iteration 4 self-review of current HEAD. Overall assessment: safe to merge.
No P0/P1/P2/P3 findings. Re-ran targeted coverage for escaped brace expansion and interp allowed-symbol compliance; all passed.
|
Iteration 4 self-review result: P0=0, P1=0, P2=0, P3=0. Summary: targeted escaped-brace and allowlist checks passed; no findings. |
|
@codex review this PR |
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
Iteration 5 self-review of current HEAD. Overall assessment: safe to merge.
No P0/P1/P2/P3 findings. The diff is unchanged since the previous clean review; escaped brace handling and CI allowlist coverage remain adequate.
|
Iteration 5 self-review result: P0=0, P1=0, P2=0, P3=0. Summary: diff unchanged from prior clean review; no findings. |
|
@codex review this PR |
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
Iteration 6 self-review of current HEAD. Overall assessment: safe to merge.
No P0/P1/P2/P3 findings. CI is green at the time of review, the branch is unchanged, and prior escaped-brace/allowlist verification remains valid.
|
Iteration 6 self-review result: P0=0, P1=0, P2=0, P3=0. Summary: CI is green, branch unchanged, no findings. |
Review-Fix Loop Summary
Iteration log
Final state
Remaining issues (if any)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae68608f55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| parts = append(parts, word.Parts[:i]...) | ||
| } | ||
| if parts != nil { | ||
| parts = append(parts, litParts...) |
There was a problem hiding this comment.
Preserve unchanged literals after protected braces
When a word has a protected escaped brace before a non-literal part and then a normal literal, e.g. X=x; printf '<%s>\n' \{${X}foo, the first literal makes parts non-nil, but the later unchanged foo literal has changed == false and litParts == nil, so this append drops it. Bash expands that word to {xfoo}, while this commit returns {x}, so suffix literals after parameter/quoted/command substitutions are silently lost.
Useful? React with 👍 / 👎.
|
Upstream follow-up opened to move the escaped-brace compatibility fix into mvdan/sh itself: mvdan/sh#1330 If that lands in a future mvdan.cc/sh/v3 release, rshell should be able to drop most/all of the local escaped-brace preprocessing shim in |
Summary: Dependency update — 1 package upgraded (patch changes only)
Manifests changed:
.(go)✅ Action Required: Please review the changes below. If they look good, approve and merge this PR.
Updates
Packages marked with "-" are updated due to dependency constraints.
Compatibility note
This PR also includes a small rshell compatibility shim in
interp/runner_expand.gobecause themvdan.cc/sh/v3patch upgrade changedexpand.Fieldsbehavior for escaped left braces.With
v3.13.0, rshell matched bash for inputs such as:After upgrading to
v3.13.1, escaped{could be treated as an unmatched brace bysyntax.SplitBraces, causing the backslash to be preserved or brace expansion to run when bash would treat the brace as quoted. The shim preserves bash-compatible behavior by protecting odd-backslash-escaped left braces before delegating toexpand.Fields.Coverage added:
interp/runner_expand_test.gotests/scenarios/shell/var_expand/quoting/escaped_left_brace.yamlReview Checklist
Standard review:
Update Mode: Routine Update
🤖 Generated by DataDog Automated Dependency Management System