Skip to content

Commit ee29b1e

Browse files
committed
fix issues pointed by the ai reviewer
1 parent 6acc503 commit ee29b1e

1 file changed

Lines changed: 49 additions & 26 deletions

File tree

src/makim/core.py

Lines changed: 49 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,16 @@ def _call_shell_app(
216216

217217
try:
218218
p.wait()
219+
# Explicitly check exit code for defense in depth
220+
returncode = getattr(p, 'returncode', None)
221+
if returncode and returncode != 0:
222+
MakimLogs.raise_error(
223+
f'Command exited with code {returncode}',
224+
MakimError.SH_ERROR_RETURN_CODE,
225+
returncode,
226+
exit_on_error=exit_on_error,
227+
)
228+
return False
219229
return True
220230
except xh.ErrorReturnCode as e:
221231
MakimLogs.raise_error(
@@ -826,36 +836,49 @@ def _process_hooks(
826836
all_hooks_succeeded = True
827837

828838
for hook_data in self.task_data['hooks'][hook_type]:
829-
env, variables = makim_hook._load_scoped_data('task')
830-
for k, v in env.items():
831-
os.environ[k] = v
832-
makim_hook.env_scoped = deepcopy(env)
833-
834-
if not self._should_run_hook(
835-
hook_data,
836-
original_args_clean,
837-
variables,
838-
makim_hook.env_scoped,
839-
):
840-
continue
839+
# Snapshot environment before hook execution
840+
prev_env = os.environ.copy()
841841

842-
args_hook = self._prepare_hook_args(
843-
hook_data, makim_hook, original_args_clean, args_hook_original
844-
)
842+
try:
843+
env, variables = makim_hook._load_scoped_data('task')
844+
for k, v in env.items():
845+
os.environ[k] = v
846+
makim_hook.env_scoped = deepcopy(env)
847+
848+
if not self._should_run_hook(
849+
hook_data,
850+
original_args_clean,
851+
variables,
852+
makim_hook.env_scoped,
853+
):
854+
continue
845855

846-
hook_success = self._execute_hook(makim_hook, args_hook, hook_data)
856+
args_hook = self._prepare_hook_args(
857+
hook_data,
858+
makim_hook,
859+
original_args_clean,
860+
args_hook_original,
861+
)
847862

848-
if not hook_success:
849-
hook_task_name = hook_data['task']
850-
hook_task_copy = deepcopy(makim_hook)
851-
hook_task_copy._change_task(hook_task_name)
852-
hook_ignore_errors = hook_task_copy.task_data.get(
853-
'options', {}
854-
).get('ignore-errors', False)
863+
hook_success = self._execute_hook(
864+
makim_hook, args_hook, hook_data
865+
)
855866

856-
if not hook_ignore_errors:
857-
all_hooks_succeeded = False
858-
break
867+
if not hook_success:
868+
hook_task_name = hook_data['task']
869+
hook_task_copy = deepcopy(makim_hook)
870+
hook_task_copy._change_task(hook_task_name)
871+
hook_ignore_errors = hook_task_copy.task_data.get(
872+
'options', {}
873+
).get('ignore-errors', False)
874+
875+
if not hook_ignore_errors:
876+
all_hooks_succeeded = False
877+
break
878+
finally:
879+
# Restore environment after hook execution
880+
os.environ.clear()
881+
os.environ.update(prev_env)
859882
return all_hooks_succeeded
860883

861884
def _should_run_hook(

0 commit comments

Comments
 (0)