Fix quoted argument parsing in agent-check.mjs with escape handling#45
Fix quoted argument parsing in agent-check.mjs with escape handling#45Copilot wants to merge 3 commits intocodex/optimize-bleedy-environment-configurationfrom
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughA new helper function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks✅ Passed checks (3 passed)
Note Free review on us!CodeRabbit is offering free reviews until Fri Nov 07 2025 to showcase some of the refinements we've made. Comment |
Co-authored-by: danelkay93 <24777308+danelkay93@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves the argument parsing in the agent-check.mjs script by adding support for quoted strings. The change allows users to pass complex test arguments that contain spaces to Vitest without breaking them into multiple arguments.
- Added a
parseQuotedArgsfunction to properly parse quoted strings - Updated
--test-args=parsing to use the new function instead of naivesplit(' ')
| function parseQuotedArgs(str) { | ||
| const result = []; | ||
| let current = ''; | ||
| let inQuote = null; | ||
|
|
||
| for (let i = 0; i < str.length; i++) { | ||
| const char = str[i]; | ||
|
|
||
| if (inQuote) { | ||
| if (char === inQuote) { | ||
| inQuote = null; | ||
| } else { | ||
| current += char; | ||
| } | ||
| } else if (char === '"' || char === "'") { | ||
| inQuote = char; | ||
| } else if (char === ' ') { | ||
| if (current) { | ||
| result.push(current); | ||
| current = ''; | ||
| } | ||
| } else { | ||
| current += char; | ||
| } | ||
| } | ||
|
|
||
| if (current) { | ||
| result.push(current); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
The function doesn't handle unclosed quotes. If a string ends with an unclosed quote (e.g., --test-args='--run --coverage), the function will silently accept it and return the partial result. Consider adding validation to detect and report unclosed quotes, or at least push the current buffer even when inQuote is still set at the end.
There was a problem hiding this comment.
Fixed in b9455e8. Added validation that throws an error with a clear message when quotes are unclosed, e.g., Error: Unclosed quote in arguments: expected closing ".
| if (inQuote) { | ||
| if (char === inQuote) { | ||
| inQuote = null; | ||
| } else { | ||
| current += char; | ||
| } |
There was a problem hiding this comment.
The parser doesn't support escaped quotes within quoted strings. For example, --test-args="test \"name\"" would incorrectly close the quote at the first escaped quote. Consider handling backslash escapes to support cases like \" and \' within quoted strings.
There was a problem hiding this comment.
Fixed in b9455e8. Added support for backslash-escaped quotes. Now --test-args="test \"name\"" correctly parses as ['test "name"'].
|
@copilot rebase to resolve conflicts |
|
@coderabbitai configuration |
CodeRabbit configurationlanguage: en-US
tone_instructions: ''
early_access: true
enable_free_tier: true
reviews:
profile: chill
request_changes_workflow: true
high_level_summary: true
high_level_summary_placeholder: '@coderabbitai summary'
high_level_summary_in_walkthrough: true
auto_title_placeholder: '@coderabbitai'
auto_title_instructions: ''
review_status: true
commit_status: true
fail_commit_status: true
collapse_walkthrough: true
changed_files_summary: true
sequence_diagrams: true
estimate_code_review_effort: true
assess_linked_issues: true
related_issues: true
related_prs: true
suggested_labels: true
labeling_instructions: []
auto_apply_labels: true
suggested_reviewers: true
auto_assign_reviewers: true
in_progress_fortune: true
poem: true
path_filters: []
path_instructions: []
abort_on_close: false
disable_cache: false
auto_review:
enabled: true
auto_incremental_review: true
ignore_title_keywords: []
labels: []
drafts: true
base_branches:
- .*
ignore_usernames: []
finishing_touches:
docstrings:
enabled: true
unit_tests:
enabled: true
pre_merge_checks:
docstrings:
mode: warning
threshold: 80
title:
mode: warning
requirements: ''
description:
mode: warning
issue_assessment:
mode: warning
custom_checks: []
tools:
ast-grep:
rule_dirs: []
util_dirs: []
essential_rules: true
packages: []
shellcheck:
enabled: true
ruff:
enabled: true
markdownlint:
enabled: true
github-checks:
enabled: true
timeout_ms: 90000
languagetool:
enabled: true
enabled_rules: []
disabled_rules: []
enabled_categories: []
disabled_categories: []
enabled_only: false
level: default
biome:
enabled: true
hadolint:
enabled: true
swiftlint:
enabled: true
phpstan:
enabled: true
level: default
phpmd:
enabled: true
phpcs:
enabled: true
golangci-lint:
enabled: true
yamllint:
enabled: true
gitleaks:
enabled: true
checkov:
enabled: true
detekt:
enabled: true
eslint:
enabled: true
flake8:
enabled: true
rubocop:
enabled: true
buf:
enabled: true
regal:
enabled: true
actionlint:
enabled: true
pmd:
enabled: true
clang:
enabled: true
cppcheck:
enabled: true
semgrep:
enabled: true
circleci:
enabled: true
clippy:
enabled: true
sqlfluff:
enabled: true
prismaLint:
enabled: true
pylint:
enabled: true
oxc:
enabled: true
shopifyThemeCheck:
enabled: true
luacheck:
enabled: true
brakeman:
enabled: true
dotenvLint:
enabled: true
htmlhint:
enabled: true
checkmake:
enabled: true
osvScanner:
enabled: true
chat:
art: true
auto_reply: true
integrations:
jira:
usage: auto
linear:
usage: auto
knowledge_base:
opt_out: false
web_search:
enabled: true
code_guidelines:
enabled: true
filePatterns: []
learnings:
scope: global
issues:
scope: auto
jira:
usage: auto
project_keys: []
linear:
usage: auto
team_keys: []
pull_requests:
scope: auto
mcp:
usage: enabled
disabled_servers: []
code_generation:
docstrings:
language: en-US
path_instructions: []
unit_tests:
path_instructions: []
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/agent-check.mjs (1)
9-46: Consider using a well-tested argument parsing library.Parsing quoted command-line arguments is a solved problem with edge cases (escaped quotes, nested quotes, etc.) already handled by established libraries. Consider using
string-argvorshell-quoteinstead of a custom implementation.Example with
string-argv:+import parseArgsString from 'string-argv'; + -/** - * Parse a string into arguments, respecting quoted substrings. - * Handles both single and double quotes. - * @param {string} str - The string to parse - * @returns {string[]} Array of parsed arguments - */ -function parseQuotedArgs(str) { - const result = []; - let current = ''; - let inQuote = null; - - for (let i = 0; i < str.length; i++) { - const char = str[i]; - - if (inQuote) { - if (char === inQuote) { - inQuote = null; - } else { - current += char; - } - } else if (char === '"' || char === "'") { - inQuote = char; - } else if (char === ' ') { - if (current) { - result.push(current); - current = ''; - } - } else { - current += char; - } - } - - if (current) { - result.push(current); - } - - return result; -}Then update line 55:
- passthroughArgs.push(...parseQuotedArgs(argsString)); + passthroughArgs.push(...parseArgsString(argsString));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/agent-check.mjs(1 hunks)
🔇 Additional comments (2)
scripts/agent-check.mjs (2)
23-28: Verify: Should unclosed quotes produce a warning or error?If a quote is never closed, the function silently consumes the rest of the string into the current token. This could lead to confusing behavior for users.
Example:
--test-args='--grep "unclosed test' # Results in: ['--grep', 'unclosed test']Should the function warn users about unclosed quotes, or is the current silent behavior acceptable?
Also applies to: 41-43
54-55: Clean integration of the new parser.The usage of
parseQuotedArgscorrectly extracts and parses the test arguments. The spread operator properly adds all parsed arguments to thepassthroughArgsarray.
| function parseQuotedArgs(str) { | ||
| const result = []; | ||
| let current = ''; | ||
| let inQuote = null; | ||
|
|
||
| for (let i = 0; i < str.length; i++) { | ||
| const char = str[i]; | ||
|
|
||
| if (inQuote) { | ||
| if (char === inQuote) { | ||
| inQuote = null; | ||
| } else { | ||
| current += char; | ||
| } | ||
| } else if (char === '"' || char === "'") { | ||
| inQuote = char; | ||
| } else if (char === ' ') { | ||
| if (current) { | ||
| result.push(current); | ||
| current = ''; | ||
| } | ||
| } else { | ||
| current += char; | ||
| } | ||
| } | ||
|
|
||
| if (current) { | ||
| result.push(current); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
If keeping the custom parser: Add escape sequence handling.
The current implementation doesn't handle escaped quotes (e.g., \" or \'). Users cannot include the quote character within a quoted string, which is standard shell behavior.
Example that would fail:
--test-args='--grep "test \"with\" quotes"'If you prefer to keep the custom implementation, apply this diff to handle escape sequences:
function parseQuotedArgs(str) {
const result = [];
let current = '';
let inQuote = null;
+ let escaped = false;
for (let i = 0; i < str.length; i++) {
const char = str[i];
+ if (escaped) {
+ current += char;
+ escaped = false;
+ continue;
+ }
+
+ if (char === '\\') {
+ escaped = true;
+ continue;
+ }
+
if (inQuote) {
if (char === inQuote) {
inQuote = null;
} else {
current += char;
}
} else if (char === '"' || char === "'") {
inQuote = char;
} else if (char === ' ') {
if (current) {
result.push(current);
current = '';
}
} else {
current += char;
}
}
if (current) {
result.push(current);
}
return result;
}🤖 Prompt for AI Agents
In scripts/agent-check.mjs around lines 15 to 46, the custom parseQuotedArgs
function doesn't handle backslash escape sequences so quoted strings can't
contain escaped quotes or backslashes (e.g., \" or \\) and escaped spaces
outside quotes; update the parser to treat a backslash as an escape: when
encountering '\' consume the next character and append it to current (both
inside and outside quotes), ensure escaped quotes don't toggle inQuote,
correctly handle escaped backslashes, and guard against end-of-string after a
backslash so you don't read past the string.
There was a problem hiding this comment.
@gemini @copilot Ditch this custom implemented parser for a modern, clean well-maintained and popular library or package. Maybe this script and functionality in its entirety should be Python-based, as indicated (or was indicated) in the conventions for this repo, to avoid bash and js devops code horrors when possible, defaulting to Python, Pulumi, and Plumbum (or more modern alternative) when applicable.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| } else if (char === '"' || char === "'") { | ||
| inQuote = char; | ||
| } else if (char === ' ') { | ||
| if (current) { | ||
| result.push(current); | ||
| current = ''; | ||
| } |
There was a problem hiding this comment.
Empty quoted strings are silently ignored.
When encountering empty quotes like "" or '', the parser adds nothing to the result array. This may not be the intended behavior, as empty strings can be meaningful in some contexts (e.g., --output="").
🤖 Prompt for AI Agents
In scripts/agent-check.mjs around lines 29-35, the parser currently ignores
empty quoted tokens ("" or ''), so when a closing quote is encountered the empty
string isn't added to result; update the quote-handling logic so that when a
closing quote is seen (inQuote === char) you clear inQuote, push the current
token even if it's an empty string, and reset current to '' (instead of only
pushing non-empty current on spaces), ensuring empty quoted strings are
preserved as tokens like --output="".
Co-authored-by: danelkay93 <24777308+danelkay93@users.noreply.github.com>
I've addressed the code review feedback in b9455e8. However, I don't see a base branch to rebase onto in the repository. Could you provide the name of the branch you'd like me to rebase onto? The repository currently only shows |
@copilot Commit this as the contents of a CodeRabbit configuration file |
I'd be happy to create a CodeRabbit configuration file. However, I don't see the full content you'd like me to commit. Could you provide the complete configuration content? CodeRabbit typically uses |
@copilot If applicable, do this |
I've already addressed all visible code review feedback in commit b9455e8 (unclosed quotes and escaped quotes handling). Could you provide the specific 2 actionable comments you're referring to? I don't see them in the current PR conversation. |
@copilot Commit this to the repo in a new PR as a @coderabbitai configuration YAML |
I don't see the CodeRabbit configuration YAML content in your comment. Could you provide the complete YAML configuration you'd like me to commit? Please paste the full content and I'll create the configuration file in a new PR. |
|
@copilot |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 207 🏁 Script executed: Length of output: 28 @danelkay93, I cannot commit code directly to the repository, but I can guide you on how to add the CodeRabbit configuration file yourself. To save your CodeRabbit configuration, you'll need to create a Option 1: Via GitHub UI
Option 2: Via Command Line# Create the configuration file in the repository root
cat > .coderabbit.yaml << 'EOF'
language: en-US
tone_instructions: ''
early_access: true
enable_free_tier: true
reviews:
profile: chill
request_changes_workflow: true
# ... (rest of your configuration)
EOF
# Add and commit the file
git add .coderabbit.yaml
git commit -m "Add CodeRabbit configuration"
git pushHow CodeRabbit Uses This ConfigurationOnce the
The configuration you've provided enables a comprehensive set of linting tools (shellcheck, ruff, markdownlint, biome, etc.) and sets the review profile to "chill" mode with various finishing touches enabled. Would you like help understanding any specific configuration options, or would you prefer to adjust any settings before creating the file? |
|
Tip For best results, initiate chat on the files or code changes. @danelkay93, I notice this comment is addressed to Just to clarify: I'm CodeRabbit, and I cannot commit code directly to the repository or create pull requests. This is by design for security reasons. However, I can help you understand how CodeRabbit configuration works: The configuration you've posted is already active! When you ran the If you want to:
The YAML configuration you've shown is comprehensive and already controlling how I review your PRs, so no additional action is needed unless you want to make changes! 🐰 🧠 Learnings used |
Addresses Sourcery AI feedback on PR #41: splitting
--test-argsby spaces breaks quoted arguments.Changes
parseQuotedArgs()function that respects single and double quotessplit(' ')with proper quote-aware parsing\"and\')Examples
Before:
After:
Escaped quotes:
Error handling:
Enables passing complex test arguments like
--test-args='--testNamePattern "integration tests" --coverage'without breaking quoted substrings, while properly handling edge cases like escaped quotes and malformed input.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.