Fix R run argument quoting

[fallintoplace]

Summary

Updates the R run() path to build Rscript arguments as an argv vector and execute them through system2(), with shell quoting applied at the execution boundary.

Issue

No existing issue.

Reproduction

Runtime: local R binding

Commands to run:

cd R
Rscript -e 'testthat::test_file("tests/testthat/test-run.R")'
Rscript -e 'testthat::test_file("tests/testthat/test-flags.R")'
Rscript -e 'testthat::test_file("tests/testthat/test-sfn-cli-parsing.R")'

Where evidence shows up: testthat output from the focused R tests.

Before

run_cmd() assembled one command string with paste(), so values containing spaces, quotes, semicolons, or other shell-sensitive characters were interpreted as part of the shell command line instead of staying as single CLI arguments.

After

run_argv() keeps each CLI token as a separate argument. run() passes the quoted argv vector to system2("Rscript", ...), and run_cmd() now returns a shell-quoted display command.

Root Cause

The R wrapper built a single shell command string by concatenating the wrapper path, flow RDS path, run mode, tags, parameters, batch identifiers, and extra args. That mixed argument construction with shell parsing, so argument boundaries depended on shell syntax instead of the values passed to run().

Why This Fix Is Correct

The fix separates command construction from command rendering. run_argv() builds the intended CLI argument vector, run() quotes that vector only at the system2() boundary, and run_cmd() remains available as a quoted display string.

Failure Modes Considered

1. Values containing spaces, quotes, semicolons, $, and unicode must remain single arguments.
2. Existing R binding command shapes still need to be preserved for default runs, batch = TRUE, Step Functions commands, help, and vector-valued parameters.

Tests

• Added argv regression coverage for spaces, quotes, semicolons, $, and unicode in tags, parameters, batch user/run IDs, and other_args.
• Updated Step Functions command parsing tests to assert argv vectors instead of splitting shell strings.
• Updated the R test helper commands to use system2().
• Ran git diff --check locally.
• Could not run the focused R tests locally because Rscript is not installed in this environment.

Non-Goals

• This does not change the Python CLI behavior.
• This does not rewrite the R command-line parser beyond preserving argument boundaries for the command that run() executes.

[greptile-apps]

Greptile Summary

This PR refactors the R binding's subprocess invocation from a single pasted shell string (system(run_cmd(...))) to a proper argv vector (system2("Rscript", shell_quote_args(run_argv(...)))), preventing shell-injection of spaces, quotes, semicolons, $, and unicode in tags, parameters, and batch identifiers. It also fixes a latent crash where resume = FALSE left the run variable uninitialized.

• run_cmd is split into run_argv (returns a character vector of arguments) and a display-only run_cmd (returns a shell-quoted string); new helpers as_cli_args, compact_args, and shell_quote_args keep arg construction and shell rendering separate.
• split_parameter_args replaces the old split_parameters string builder, emitting interleaved c("--name", "value") pairs suitable for direct use in an argv vector.
• Tests are updated to assert on argv vectors rather than splitting shell strings, and new regression cases cover shell-sensitive values end-to-end through run_argv.

Confidence Score: 3/5

Safe to merge for shell-quoting improvements at the invocation boundary, but split_flags in the subprocess still splits on spaces, so space-containing tag/parameter values will be silently corrupted end-to-end.

The invocation-boundary fix is correct and well-tested at the run_argv level, but split_flags re-splits every argv token on spaces inside the subprocess. A tag like "has spaces" is delivered correctly by the OS, then broken by split_flags. The new tests only exercise run_argv() directly and never run a real subprocess, so the gap is invisible in CI.

R/R/flags.R — the split_flags function at line 131 undermines the space-safety guarantees the rest of the PR establishes.

Important Files Changed

Filename	Overview
R/R/run.R	Core change: run_cmd split into run_argv (returns argv vector) and updated run_cmd (returns shell-quoted display string); run() now uses system2 with shell_quote_args; new helpers as_cli_args, compact_args, shell_quote_args added; fixes missing else { run <- "run" } branch for resume=FALSE
R/R/flags.R	split_parameters now wraps new split_parameter_args (returns argv-style vector); parameter formatting now emits c("--name", "value") pairs instead of pasting a single string; but split_flags still splits each token on spaces, which will re-break space-containing values passed from the subprocess argv
R/tests/testthat/test-run.R	Tests updated from run_cmd string-splitting to run_argv vector assertions; new tests cover shell-sensitive values in tags, parameters, and batch identifiers; tests only verify the argv vector, not end-to-end subprocess parsing
R/tests/testthat/test-flags.R	CLI test helper updated from system() to system2(); new tests for split_parameter_args covering shell-sensitive values and vector-valued parameters
R/tests/testthat/test-sfn-cli-parsing.R	All SFN tests rewritten with shared read_run_argv helper; tests now assert argv vectors instead of re-joined strings; cleanup consolidated into helper
R/tests/testthat/test-run-cmd.R	Helper script updated to call run_argv instead of run_cmd and save the argv vector

Comments Outside Diff (1)

1. R/R/flags.R, line 131-136 (link)

   split_flags re-splits space-containing argv tokens

   split_flags applies strsplit(x, split = " ") to every element of commandArgs(trailingOnly = TRUE) inside the subprocess. When run() invokes the subprocess via system2 with correctly separated tokens (e.g., "--tag" and "has spaces"), the OS delivers them as two distinct elements to commandArgs. split_flags then splits "has spaces" into c("has", "spaces"), so parse_arguments receives three tokens and picks up "has" as the tag value, silently dropping "spaces".

   The PR fixes the invocation boundary but this function undermines the fix for any value that contains a space. The new tests in test-run.R only assert on the run_argv return value; they do not run the subprocess end-to-end, so the breakage is not caught by the new test suite.

_{Reviews (1): Last reviewed commit: "Fix R run argument quoting" | Re-trigger Greptile}

[greptile-apps]

shell_quote_args is redundant when system2 already handles quoting on some platforms

system2 on POSIX (with the default stdout = "", stderr = "") joins the command and args into a shell string, so shQuote is correct and necessary here. However, on Windows system2 passes each element of args directly to CreateProcess and applies its own quoting; pre-quoting with shQuote would cause the double-quote characters to be treated as literal content by the child process.

The practical risk is low (metaflow's R binding primarily targets POSIX), and the old system(cmd) path was equally broken on Windows, so this is not a regression. Consider documenting the POSIX-only assumption in a comment, or passing args unquoted and setting stdout = NULL, stderr = NULL, stdin = NULL (which causes R to use execvp directly on POSIX, where no shell quoting is needed).

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[LuisJG8]

Hi @fallintoplace, fyi the R integration is basically deprecated so we are not accepting any contributions to it.

[agsaru]

R module is deprecated so changes here do not matter anymore. Confirmed by the admin.