Have python return status code

[Alex-Omar-Nvidia]

Description

The develop branch changes the way the python interpreter is called and no longer returns the python status code.

On main, python exit codes are bubbled up when the process ends. In develop this was removed during a refactor. This MR adds this functionality back.

Type of change

• Bug fix (non-breaking change which fixes an issue)

Screenshots

Please attach before and after screenshots of the change if applicable.

Current Behavior On Develop Branch

# python interpreter
import sys
sys.exit(1)

# bash
echo $?
> 0

Current Behavior on Main Branch

# python interpreter
import sys
sys.exit(1)

# bash
echo $?
> 1

Behavior with this MR

# python interpreter
import sys
sys.exit(1)

# bash
echo $?
> 1

Fixes # 5237

Checklist

• I have read and understood the contribution guidelines
• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have updated the changelog and the corresponding version in the extension's config/extension.toml file
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[greptile-apps]

Greptile Summary

Restores Python subprocess exit-code propagation for the -p/--python CLI flag, which was silently dropped during the develop branch refactor. The fix passes check=True to run_python_command, causing run_command's CalledProcessError handler to call sys.exit(returncode) on non-zero exits.

• The CHANGELOG has not been updated; the project guidelines require a new version entry in source/isaaclab/docs/CHANGELOG.rst and a matching bump in source/isaaclab/config/extension.toml for every source change.
• Using check=True triggers the [ERROR] Command failed with code N: \"...\" message for every non-zero exit — including intentional ones from test scripts. Propagating the exit code directly (sys.exit(result.returncode)) would be cleaner.

Confidence Score: 4/5

Functionally correct bug fix; safe to merge after addressing the changelog requirement and optional UX improvement.

The core fix is correct and restores the expected behaviour. The open items are a missing mandatory changelog entry (project policy) and a P2 UX concern about noisy error output from the check=True path. Neither is a runtime defect, but the changelog policy is explicitly required by AGENTS.md.

source/isaaclab/docs/CHANGELOG.rst and source/isaaclab/config/extension.toml — not updated in this PR but required by project guidelines.

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/cli/init.py	Adds check=True to run_python_command calls for the -p/--python handler, restoring exit code propagation lost in the develop-branch refactor; functionally correct but routes through the error handler, printing a noisy [ERROR] message on any non-zero exit.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["isaaclab -p script.py"] --> B["cli(): args.python is not None"]
    B --> C["run_python_command(script, args, check=True)"]
    C --> D["run_command(cmd, check=True)"]
    D --> E["subprocess.run(check=True)"]
    E -->|exit code 0| F["Returns CompletedProcess"]
    E -->|exit code != 0| G["Raises CalledProcessError"]
    F --> H["cli() returns → process exits 0"]
    G --> I["print_error('Command failed...')"]
    I --> J["sys.exit(returncode)"]
    J --> K["Shell sees correct exit code"]
    H --> L["Shell sees exit code 0"]

Loading

_{Reviews (1): Last reviewed commit: "Have python return status code" | Re-trigger Greptile}

[greptile-apps]

check=True adds spurious error output for intentional non-zero exits

Using check=True routes through run_command's CalledProcessError handler, which prints [ERROR] Command failed with code N: "..." before calling sys.exit(N). This is noisy for Python scripts that intentionally call sys.exit(1) (e.g., test runners, CI scripts). A cleaner approach is to use check=False and propagate the return code explicitly:

Suggested change

	run_python_command(args.python[0], args.python[1:], check=True)
	else:
	run_python_command("-i", [])
	run_python_command("-i", [], check=True)
	result = run_python_command(args.python[0], args.python[1:])
	sys.exit(result.returncode)
	else:
	result = run_python_command("-i", [])
	sys.exit(result.returncode)

This propagates the exit code for all cases (including 0) without the extra error message.

[isaaclab-review-bot]

Review by IsaacLab PR Bot 🤖

Summary

Clean, minimal bug fix that restores exit code propagation for isaaclab.sh -p / --python commands. The regression was introduced during a refactor on develop where run_python_command defaulted check=False, causing non-zero exit codes from Python scripts to be silently swallowed (always returning 0).

Analysis

The fix is correct: passing check=True makes run_command → subprocess.run raise CalledProcessError on non-zero exits, which run_command's exception handler then propagates via sys.exit(e.returncode).

Minor nit (non-blocking)

The Greptile bot raised a fair point: with check=True, the run_command error handler prints [ERROR] Command failed with code N: "..." before exiting. For scripts that intentionally use sys.exit(1) (test runners, CI tools, etc.), this extra error line is a bit noisy. A slightly cleaner approach would be:

result = run_python_command(args.python[0], args.python[1:])
sys.exit(result.returncode)

This propagates the exit code in all cases without the spurious error message. That said, the current approach is functionally correct and the error message is minor — I'd leave it to the author's preference.

Scope consideration

Note that run_python_command is also called in commands/misc.py (for --test, --new, --docs, --vscode) where check also defaults to False. Those callers might have the same silent-failure behavior, but they're out of scope for this issue (#5237) which specifically targets --python. Worth a follow-up if desired.

Verdict

✅ LGTM — correct fix, well-scoped, good PR description with before/after evidence. Ship it.

[myurasov-nv]

Please rebase on develop to resolve some of the CI failures, there has been changes addressing them.

[myurasov-nv]

I agree that this is ok to add. Please rebase and merge.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

Clean, minimal bug fix that restores Python exit code propagation when using isaaclab.sh -p / --python. The regression was introduced during a refactor on develop where run_python_command was changed to default check=False, causing subprocess.run to silently discard non-zero exit codes. Adding check=True at the two call sites restores the original behavior: run_command catches the resulting CalledProcessError and calls sys.exit(e.returncode), correctly bubbling the exit code to the shell.

Design Assessment

Design is sound. The fix is targeted and correct. Rather than changing the default of check in run_python_command's signature (which could affect other callers), the PR explicitly opts in at the two --python call sites. This is the conservative, safe approach.

Findings

🟡 Warning: CHANGELOG category should be "Fixed" not "Changed" — source/isaaclab/docs/CHANGELOG.rst:4

This PR is a bug fix (restoring broken behavior, linked to issue #5237). The project's changelog convention uses "Fixed" for bug fixes (see 4.6.5, 4.6.4) and "Changed" for intentional behavior modifications. Since this restores the exit code propagation that was accidentally lost during a refactor, "Fixed" is the accurate category.

🔵 Suggestion: Same issue may affect other CLI subcommands — source/isaaclab/isaaclab/cli/__init__.py:146

Other run_python_command callers in misc.py (command_test, command_new, command_run_docker, command_vscode_settings) also use the default check=False, meaning they would similarly swallow non-zero exit codes. This is out of scope for this PR, but worth a follow-up to audit whether those commands should also propagate exit codes — particularly command_test where pytest's non-zero exit indicates test failures.

Test Coverage

• Bug fix: No regression test included, but this is an infrastructure/CLI change affecting shell exit code behavior, which is difficult to test in a standard pytest suite without process-level test harness.
• Manual testing: The PR description includes clear before/after screenshots demonstrating the fix works.
• Existing coverage: The fix is a two-character change (check=True) flowing through well-tested subprocess.run semantics — the risk of regression is minimal.

CI Status

Core checks passing (pre-commit ✅, Build Wheel ✅). Some downstream checks still pending (Installation Tests, license-check, Docker builds).

Verdict

Ship it 🚀

Correct, minimal fix for a real regression. The only actionable suggestion is the changelog category ("Fixed" vs "Changed"), which is cosmetic. The code change itself is clean and safe.

[isaaclab-review-bot]

🟡 Warning: CHANGELOG category should be "Fixed" not "Changed"

This PR is a bug fix restoring behavior that was accidentally broken during a refactor (issue #5237). The project's changelog convention uses Fixed for bug fixes (see 4.6.5, 4.6.4 entries below) and Changed for intentional behavior modifications.

Suggested change

	Changed
	Fixed
	^^^^^

[isaaclab-review-bot]

🔵 Suggestion: Other run_python_command callers may have the same issue

This fix is correct for --python. However, other callers in misc.py (command_test, command_new, command_run_docker) also use the default check=False and would similarly swallow non-zero exit codes. Worth a follow-up audit — particularly for command_test where pytest non-zero exits indicate test failures.