Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 35 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
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 |
3d66b2e to
739e098
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Static-text handling and expansion traversal miss valid Bash semantics.
Review effort: Balanced
Findings: 3
Open (3)
What changed in this PR
Adds internal utilities for Bash rule implementations.
Changes:
- Adds static-word, command-name, and expansion helpers.
- Adds 13 focused unit tests.
| File | Description |
|---|---|
src/rules/utils.ts |
Implements shared rule helpers. |
src/rules/utils.spec.ts |
Tests helper behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| case "SingleQuotedString": | ||
| return part.value; |
There was a problem hiding this comment.
Good catch. Went with the conservative option in f81f603: getStaticText() returns null for a $'...' string that contains a backslash, and still returns the text when there are no escapes to decode. Tests added for both, plus getCommandName() on $'e\x63ho'.
| case "DoubleQuotedString": { | ||
| let result = ""; |
There was a problem hiding this comment.
Fixed in f81f603: $"..." strings now return null, with a test.
| if ( | ||
| part.type === "ParameterExpansion" || | ||
| part.type === "CommandSubstitution" | ||
| ) { | ||
| if (!quoted || includeQuoted) { | ||
| expansions.push(part); | ||
| } | ||
| } else if (part.type === "DoubleQuotedString") { | ||
| visitParts(part.parts, true); | ||
| } |
There was a problem hiding this comment.
Not changing the behavior here. Every nested expansion sits inside an outer expansion that is already returned, and the helper's only consumer (no-variables-in-printf-format) just asks whether the word contains any expansion, so nothing is missed today. Recursing would also make the result ambiguous for callers that report or fix each returned node, since inner and outer ranges overlap and "quoted" isn't well defined for an operand word. The doc comment did overpromise with "all expansions", so f81f603 rewords it to say nested expansions are not returned and pins that with a test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
getStaticText() now returns null for $'...' strings containing escapes and for locale-translated $"..." strings, whose runtime text can differ from the source text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Adds the shared helpers that the rule PRs above this one build on. They're internal (not exported from the package), so this PR uses
chore:and stays out of the changelog.What's included
src/rules/utils.ts:getStaticText(word)nullif it contains an expansiongetCommandName(command)null(dynamic names like$cmd, or assignment-only statements)isCommandNamed(statement, name)getExpansions(word, { includeQuoted })They're used by
no-useless-echo,no-ls-iteration,no-useless-cat,require-read-r,require-cd-guard,no-variables-in-printf-format, andno-unused-vars.Testing
13 unit tests in
src/rules/utils.spec.ts(154 total); build, lint, and format checks pass.🤖 Generated with Claude Code
Stack created with GitHub Stacks CLI • Give Feedback 💬