diff --git a/cycode/cli/files_collector/sca/base_restore_dependencies.py b/cycode/cli/files_collector/sca/base_restore_dependencies.py index 06431f72..d5167e92 100644 --- a/cycode/cli/files_collector/sca/base_restore_dependencies.py +++ b/cycode/cli/files_collector/sca/base_restore_dependencies.py @@ -40,6 +40,9 @@ def execute_commands( for command in commands: command_output = shell(command=command, timeout=timeout, working_directory=working_directory) + if command_output is None: # shell returns None when the command exited non-zero + logger.debug('Restore command failed, %s', {'command': command}) + return None if command_output: outputs.append(command_output) diff --git a/tests/cli/files_collector/sca/test_base_restore_dependencies.py b/tests/cli/files_collector/sca/test_base_restore_dependencies.py index b291a95f..7d8d8743 100644 --- a/tests/cli/files_collector/sca/test_base_restore_dependencies.py +++ b/tests/cli/files_collector/sca/test_base_restore_dependencies.py @@ -11,7 +11,7 @@ import pytest import typer -from cycode.cli.files_collector.sca.base_restore_dependencies import BaseRestoreDependencies +from cycode.cli.files_collector.sca.base_restore_dependencies import BaseRestoreDependencies, execute_commands from cycode.cli.models import Document _LOCK_FILE_NAME = 'generated.lock' @@ -68,6 +68,39 @@ def side_effect( return side_effect +class TestExecuteCommands: + """Directly test the shell-failure sentinel handling in execute_commands.""" + + def test_returns_none_when_a_command_fails(self) -> None: + """shell() returns None on non-zero exit; execute_commands must propagate None, not ''.""" + with patch(f'{_BASE_MODULE}.shell', return_value=None): + result = execute_commands([['poetry', 'lock']], timeout=30) + + assert result is None + + def test_stops_at_first_failing_command(self) -> None: + """A failure in an earlier command short-circuits; later commands do not run.""" + mock_shell = MagicMock(side_effect=[None, 'should-not-run']) + with patch(f'{_BASE_MODULE}.shell', mock_shell): + result = execute_commands([['a'], ['b']], timeout=30) + + assert result is None + assert mock_shell.call_count == 1 + + def test_empty_output_success_is_not_treated_as_failure(self) -> None: + """A successful command with empty stdout ('') must NOT be treated as a failure.""" + with patch(f'{_BASE_MODULE}.shell', return_value=''): + result = execute_commands([['poetry', 'lock']], timeout=30) + + assert result == '' + + def test_joins_successful_outputs(self) -> None: + with patch(f'{_BASE_MODULE}.shell', side_effect=['out1', 'out2']): + result = execute_commands([['a'], ['b']], timeout=30) + + assert result == 'out1\nout2' + + class TestCleanupGeneratedFile: def test_generated_lockfile_is_deleted_after_restore(self, handler: _MinimalRestoreHandler, tmp_path: Path) -> None: doc = _make_doc(tmp_path) @@ -132,6 +165,24 @@ def test_failed_command_returns_none_and_no_file_created( assert result is None assert not lock_path.exists() + def test_shell_failure_propagates_to_none_and_no_lockfile( + self, handler: _MinimalRestoreHandler, tmp_path: Path + ) -> None: + """End-to-end failure path: shell() returns None (non-zero exit) -> restore returns None. + + Regression for the stop-on-error bug: execute_commands must NOT swallow a failed + command into an empty-string success. This exercises the real execute_commands with + only shell() mocked (the layer where a non-zero exit is signalled as None). + """ + doc = _make_doc(tmp_path) + lock_path = tmp_path / _LOCK_FILE_NAME + + with patch(f'{_BASE_MODULE}.shell', return_value=None): + result = handler.try_restore_dependencies(doc) + + assert result is None, 'A failed restore command must return None so stop-on-error can fire' + assert not lock_path.exists() + def test_generated_file_content_available_in_document_after_deletion( self, handler: _MinimalRestoreHandler, tmp_path: Path ) -> None: