tasks: quote argument tokens for the shell

[ideaship]

run_ansible_in_environment joins a list of arguments with plain spaces and runs the result through subprocess.Popen(..., shell=True), so any -e value containing a shell metacharacter is parsed by /bin/sh. A tempest regex alternation is the motivating case:

$ osism apply tempest -e 'tempest_include_regex=(A|B)'
/bin/sh: 1: Syntax error: "(" unexpected

The command dies before ansible runs at all. This affects every osism apply … -e key=value whose value contains a shell metacharacter, across all workers (kolla-ansible / kubernetes / ceph-ansible included), not just tempest.

Why not the obvious one-liner

Quoting each list element with shlex.quote is wrong, and this repo already guards against it — tests/unit/tasks/test_init.py::test_run_ansible_list_multitoken_element_word_split_not_quoted, added by d01c3cb5.

Callers deliberately pack several shell words into one list element and rely on the outer shell to split them:

• commands/set.py, commands/noset.py: ["-e status=True", f"-l {host}"]
• commands/validate.py: "-e kolla_action=config_validate"
• commands/apply.py: [f"-e kolla_action={action}"] + args

and the run-<environment>.sh scripts forward args via "$@" without re-splitting, so that split is load-bearing. A per-element quote glues -e status=True into a single token and breaks -e/-l parsing.

What this does instead

Tokenize each element the way the shell would, with shlex.split(), then quote the resulting tokens.

for argument in arguments:
    try:
        quoted_arguments.extend(shlex.quote(t) for t in shlex.split(argument))
    except ValueError:
        quoted_arguments.append(argument)
joined_arguments = " ".join(quoted_arguments)

str.split() is not sufficient, which an earlier revision of this PR got wrong. An element may use quoting or a backslash to hold whitespace inside a single value, and splitting on raw whitespace cuts that value into malformed arguments:

element	/bin/sh today	str.split() + quote	shlex.split() + quote
-e foo='hello world'	[-e][foo=hello world]	[-e][foo='hello][world'] ✗	[-e][foo=hello world] ✓
foo="hello world"	[foo=hello world]	[foo="hello][world"] ✗	[foo=hello world] ✓
-e path=a\ b	[-e][path=a b]	[-e][path=a\][b] ✗	[-e][path=a b] ✓
-e status=True	[-e][status=True]	[-e][status=True] ✓	[-e][status=True] ✓
regex=(A|B)	syntax error	[regex=(A|B)] ✓	[regex=(A|B)] ✓

shlex.split() reproduces today's tokenization for quoted and escaped whitespace, so those cases are unchanged, while making metacharacters safe.

shlex.split() raises ValueError on unbalanced quoting, where /bin/sh merely fails with its own error. Letting that propagate would turn a shell-level failure into a worker traceback, so such elements are emitted verbatim and the failure mode is preserved.

Tests

Written first and watched fail. Two added:

• test_run_ansible_list_element_with_metacharacters_is_quoted — the motivating case.
• four covering single-quoted, double-quoted and backslash-escaped whitespace, plus the unbalanced-quote passthrough.
• test_run_ansible_multitoken_element_tokens_quoted_individually — pins both properties at once: a multi-token element still word-splits and a metacharacter inside one of its tokens is quoted. That is what fixes the order as split-before-quote; neither property alone would catch a regression. It also covers the gap d01c3cb5 noted in the older test, which used only single-word elements where shlex.quote is a no-op.

check	result
tests/unit/tasks/test_init.py	86 passed, including the existing word-split guard
tests/unit (full)	3154 passed, 4 pre-existing xfail
black / mypy	clean

End-to-end verification

On a live OSISM 10.2.0 cluster, the osism-ansible worker was patched with this change and restarted. The alternation that previously failed with the syntax error now selects and runs both tests:

neutron_tempest_plugin.api.test_port_forwardings...test_port_forwarding_info_in_fip_details [4.060802s] ... ok
neutron_tempest_plugin.api.test_port_forwardings...test_port_forwarding_life_cycle          [4.439480s] ... ok
 - Passed: 2
 - Failed: 0

Note for reviewers

Branched from main. There is separate in-flight work on osism/tasks/__init__.py (aborting the collection chain when a play fails) that also edits tests/unit/tasks/test_init.py — expect a possible textual conflict in the test file if both land, but no semantic overlap: that work does not touch argument joining.

🤖 Generated with Claude Code

[sourcery-ai]

Hey - I've found 1 issue

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="osism/tasks/__init__.py" line_range="190" />
<code_context>
+        # /bin/sh unquoted and the command dies with `Syntax error: "("
+        # unexpected` before ansible runs at all.
+        quoted_arguments = []
+        for argument in arguments:
+            try:
+                quoted_arguments.extend(
+                    shlex.quote(token) for token in shlex.split(argument)
+                )
+            except ValueError:
+                # Unbalanced quoting: shlex cannot tokenize it. Emit the element
+                # verbatim so /bin/sh reports the same error it reports today,
</code_context>
<issue_to_address>
**issue (bug_risk):** `shlex.split()` is called with its default `comments=False`, so it does not reproduce `/bin/sh` tokenization for an unquoted `#` that begins a shell comment. An element such as `"-e foo=bar # ignored"` is converted into `-e foo=bar '#' ignored`, whereas the previous command passed only `-e foo=bar` to the shell and discarded the remainder as a comment.

**Triggers:** When a packed argument element contains an unquoted `#` at a shell comment boundary.

**Suggested fix:** Call `shlex.split(argument, comments=True)` to match the shell's comment handling, or explicitly document and preserve the intended literal-`#` behavior.

```suggestion
                    shlex.quote(token) for token in shlex.split(argument, comments=True)
```
</issue_to_address>

Sourcery assessment

Needs a human reviewer. 1 finding to address first, and this changes how shell arguments are tokenized and quoted before invoking Ansible, so an edge case could pass a different variable, host pattern, or action and make an unintended infrastructure change. Reverting prevents further incorrect invocations but cannot undo changes an already-run Ansible command made.

Blocking findings: osism/tasks/__init__.py:190

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (bug_risk): shlex.split() is called with its default comments=False, so it does not reproduce /bin/sh tokenization for an unquoted # that begins a shell comment. An element such as "-e foo=bar # ignored" is converted into -e foo=bar '#' ignored, whereas the previous command passed only -e foo=bar to the shell and discarded the remainder as a comment.

Triggers: When a packed argument element contains an unquoted # at a shell comment boundary.

Suggested fix: Call shlex.split(argument, comments=True) to match the shell's comment handling, or explicitly document and preserve the intended literal-# behavior.

Suggested change

	shlex.quote(token) for token in shlex.split(argument)
	shlex.quote(token) for token in shlex.split(argument, comments=True)