Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a shell command concatenation edge case in the Atomic service where whitespace-only lines between a prereq block and the ability command could introduce a stray semicolon and break generated shell syntax (mitre/caldera#3097).
Changes:
- Skip whitespace-only lines in
AtomicService._concatenate_shell_commandsto prevent generating empty “commands” that get;appended. - Add a regression test intended to ensure no extra semicolon is inserted after a prereq
fi;block.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
app/atomic_svc.py |
Filters out whitespace-only command lines before concatenating shell commands to avoid stray separators. |
tests/test_atomic_svc.py |
Adds a regression test case for the whitespace-line/fi; concatenation issue. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/test_atomic_svc.py
Outdated
| commands = 'if [ -x "$(command -v ip)" ]; then : ; else apt-get install iproute2 -y; fi;\n \n ip neighbour show' | ||
| result = AtomicService._handle_multiline_commands(commands, 'sh') | ||
| assert '; ;' not in result, f"Unexpected stray semicolon in: {result!r}" | ||
| assert 'ip neighbour show' in result |
There was a problem hiding this comment.
Pull request overview
Fixes a shell command concatenation edge case where whitespace-only lines were treated as commands, producing stray ; separators and broken syntax (mitre/caldera#3097).
Changes:
- Skip whitespace-only lines in
_concatenate_shell_commandsbefore concatenation. - Add a regression test to ensure no extra semicolon is emitted between a prerequisite
fi;block and the actual ability command.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/atomic_svc.py | Filters whitespace-only command lines to prevent emitting stray ; separators during shell concatenation |
| tests/test_atomic_svc.py | Adds regression test covering the fi; + whitespace-only line + command scenario from #3097 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_handle_multiline_command_no_extra_semicolon_after_fi(self): | ||
| """Regression test for issue #3097: whitespace-only lines between prereq block | ||
| (ending with 'fi;') and the ability command must not produce a stray '; ' separator, | ||
| which resulted in commands like 'fi; ; ip neighbour show'.""" | ||
| # Simulate the precmd built by _prepare_executor: | ||
| # dep_construct ends with 'fi;', then ' \n ' (two spaces) separates it from the | ||
| # ability command — matching the double-space pattern of the actual bug report. | ||
| commands = 'if [ -x "$(command -v ip)" ]; then : ; else apt-get install iproute2 -y; fi;\n \n ip neighbour show' | ||
| result = AtomicService._handle_multiline_commands(commands, 'sh') | ||
| assert '; ;' not in result, f"Unexpected stray semicolon (double-space) in: {result!r}" | ||
| assert 'ip neighbour show' in result |
…vent stray semicolons When the prereq block (ending with 'fi;') is combined with the ability command via 'dep_construct \n command', a whitespace-only line between them caused _concatenate_shell_commands to append '; ' after it, producing commands like 'fi; ; ip neighbour show' (issue #3097 on mitre/caldera). Filtering out whitespace-only lines before concatenation eliminates the stray separator. A regression test is included.
… the actual bug
The original regression test used a single-space whitespace line (' \n ')
between the prereq block and the command, but the real bug (issue #3097)
produced 'fi; ; ip neighbour show' with double spaces. Update the test
input to ' \n ' and the assertion to check for '; ;' so the test
accurately reproduces the triggering condition.
deacon-mp
force-pushed
the
fix/issue-3097-ip-neighbour-semicolon
branch
from
db47d1b to
735ca57
Compare
There was a problem hiding this comment.
Pull request overview
Fixes generation of malformed shell commands when multiline command strings contain whitespace-only lines (e.g., between a prerequisite fi; block and the actual ability command), preventing stray ; separators and broken shell syntax.
Changes:
- Skip whitespace-only lines in
AtomicService._concatenate_shell_commandsbefore concatenation. - Add a regression test covering the
fi;+ blank line + command scenario from caldera#3097.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
app/atomic_svc.py |
Filters out whitespace-only command lines prior to joining, avoiding extra semicolon insertion. |
tests/test_atomic_svc.py |
Adds regression test to ensure no stray semicolon is produced after fi; when blank lines are present. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| commands = 'if [ -x "$(command -v ip)" ]; then : ; else apt-get install iproute2 -y; fi;\n \n ip neighbour show' | ||
| result = AtomicService._handle_multiline_commands(commands, 'sh') | ||
| assert '; ;' not in result, f"Unexpected stray semicolon (double-space) in: {result!r}" | ||
| assert 'ip neighbour show' in result |
…s pattern Replace the exact string check for '; ;' (double-space only) with a regex assertion re.search(r';\s+;', result) so the regression test catches any amount of whitespace between consecutive semicolons, not just the double-space case from the original bug report.
|
@copilot review |
|
@deacon-mp I've opened a new pull request, #56, to work on those changes. Once the pull request is ready, I'll request review from you. |
Summary
ip neighbourability command.fi;) is combined with the ability command viaf"{dep_construct} \n {command}", a whitespace-only line between them was treated as a real command line in_concatenate_shell_commands, causing'; 'to be appended after it.if [ -x "$(command -v ip)" ]; then : ; else apt-get install iproute2 -y; fi; ; ip neighbour show_concatenate_shell_commandsbefore concatenation.test_handle_multiline_command_no_extra_semicolon_after_fiis added.Test plan
python -m pytest plugins/atomic/tests/test_atomic_svc.py -vtest_handle_multiline_command_no_extra_semicolon_after_fiverifies the fixshprerequisite (e.g.ip neighbour show) and confirm the generated command no longer contains; ;🤖 Generated with Claude Code