fix(plugins): fail install loudly when setup.sh fails for privileged MCP plugin #153
Reference in New Issue
Block a user
Delete Branch "fix/151-setup-sh-fail-loud"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #151.
AgentskillsAdaptor.install()previously appended a WARNING onsetup.shnon-zero exit or timeout and reported the install as successful. Formolecule-platform-mcp(the org-management MCP plugin) this leaves the concierge with a configured-but-missingmolecule-mcpbinary and no loud failure.Changes
errorsfield toInstallResultso callers can observe hard failures that did not raise.setup.shfailure as both a warning (existing contract) and an error.molecule-platform-mcp, raiseRuntimeErroronsetup.shfailure so the install cannot succeed silently.adapter_base.install_plugins_via_registry().Tests
tests/test_plugins_setup_sh_failure.pycovering ordinary plugin records error, privileged plugin raises, and success has no errors.626 passed, 1 skipped.Fixes #151
🤖 Generated with Claude Code
REQUEST_CHANGES. 5-axis review found one blocking test-coverage gap for the requested failure surface. Correctness/robustness: the implementation does add result.errors for setup.sh non-zero exits and TimeoutExpired, and privileged molecule-platform-mcp raises RuntimeError on both paths. The non-zero and success paths are covered. However, #151 and the review request specifically require timeout behavior too, and tests/test_plugins_setup_sh_failure.py does not exercise subprocess.TimeoutExpired for either ordinary plugins or the privileged plugin. For this privileged MCP install path, timeout is one of the two silent-success hazards being fixed, so please add focused tests that monkeypatch subprocess.run to raise subprocess.TimeoutExpired and assert: ordinary plugin records an error/warning, privileged molecule-platform-mcp raises RuntimeError. Security: the direction is right because it prevents configured-but-missing privileged MCP binaries, but timeout regression coverage should be present before approval. Performance/readability: no concerns with the production code; the tests can avoid sleeping by monkeypatching. CI is green, but this missing coverage is a blocker for this change.
5-axis review: REQUEST_CHANGES.
Correctness blocker: the privileged adaptor now raises on
setup.shnon-zero exit/timeout, but the caller still swallows that exception.molecule_runtime/plugins_registry/builtins.py:253-261raisesRuntimeErrorformolecule-platform-mcp, thenmolecule_runtime/adapter_base.py:466-480catches every exception fromadaptor.install(ctx), logs it, does not append a failedInstallResult, and continues. That means the runtime-level install/setup path can still complete after the privileged MCPsetup.shfails, which does not satisfy “fail install loudly” or prevent a configured-but-missing MCP binary state.Robustness/test gap: tests cover ordinary non-zero exit, privileged non-zero exit, and success, but they do not cover the timeout path. The production timeout branch at
builtins.py:255-261should be covered, and the runtime-level caller behavior above needs a regression test so a raised privileged setup failure cannot be silently downgraded to a log line again.Security/performance/readability: no separate concerns in the changed code; the scope is otherwise small and CI is green. The required fix shape is in
molecule_runtime/adapter_base.pyplus tests: propagate or otherwise surface privileged install failures as a hard setup/install failure, and add timeout coverage for the privileged path.Addressed both review requests in
7c31560:test_setup_sh_timeout_records_error(ordinary plugin) andtest_setup_sh_timeout_on_privileged_plugin_raises(molecule-platform-mcp) that monkeypatchsubprocess.runto raiseTimeoutExpired.BaseAdapter.install_plugins_via_registrynow re-raises the install exception whenplugin.name == _PRIVILEGED_MCP_PLUGIN, so a privileged setup failure cannot be downgraded to a log line. Addedtests/test_adapter_base_plugin_failure.pyto regression-cover both privileged-propagation and non-privileged-swallow behavior.CI:
pytest tests/test_plugins*.py tests/test_adapter_base_plugin_failure.pypasses locally.5-axis re-review on new head
7c315603: APPROVED.Correctness/robustness: the prior REQUEST_CHANGES finding is resolved.
molecule_runtime/plugins_registry/builtins.py:249-261still recordssetup.shnon-zero exits and timeouts as warnings+errors, and raises for the privilegedmolecule-platform-mcpplugin.molecule_runtime/adapter_base.py:479-482now re-raises adaptor install exceptions when the plugin is privileged, so the runtime-level install/setup path no longer swallows the privileged setup failure. Non-privileged plugin failures keep the prior continue/log behavior, and non-privilegedsetup.shfailures still return anInstallResultwith errors instead of aborting.Tests:
tests/test_adapter_base_plugin_failure.pyexercises the adapter boundary: privileged install exceptions propagate throughinstall_plugins_via_registry(), while ordinary plugin exceptions are swallowed.tests/test_plugins_setup_sh_failure.pynow covers non-zero exit, success, ordinary timeout recording, and privileged timeout raising. CI is green across lint/build/smoke/unit/e2e/secret-scan.Security/performance/readability: no remaining concerns. The change is scoped to the plugin install failure path and makes the privileged MCP setup failure loud as intended.
Fresh re-review on head
7c315603. APPROVED.I rechecked the setup.sh failure handling across the five-axis review. The non-zero exit and timeout paths now both surface install errors, preserve the ordinary-plugin error reporting behavior, and fail loudly for the privileged molecule-platform-mcp plugin. The new timeout tests exercise TimeoutExpired for both ordinary and privileged plugins, and the adapter-level tests cover privileged propagation while preserving non-privileged failure logging/swallowing behavior. Success path remains covered and unchanged. CI statuses on this head are success.
New commits pushed, approval review dismissed automatically according to repository settings
New commits pushed, approval review dismissed automatically according to repository settings
5-axis re-review on new head
b95b0b0c: REQUEST_CHANGES.Correctness: the production code path now appears to close the loud-failure gap.
molecule_runtime/adapter_base.pydefinesPrivilegedPluginInstallErrorand re-raises privileged-plugin install failures,builtins.pyraises that dedicated type for privilegedsetup.shnon-zero exit/timeout, andmolecule_runtime/main.py:477-491now detectsPrivilegedPluginInstallErrorinside the outer setupexceptand re-raises instead of degrading to reachable-but-misconfigured. Ordinary exceptions still fall through to the existing warning/degraded boot path, so ordinary plugins retain record/log-and-continue behavior.Blocking test issue:
tests/test_main_privileged_plugin_failure.pydoes not actually exercisemain.pyor the entrypoint catch block. It defines_outer_except_handler()inside the test file and duplicates the intendedisinstance(setup_err, PrivilegedPluginInstallError)logic, then tests that duplicate. Ifmain.pylater drifts back to swallowingPrivilegedPluginInstallError, these tests would still pass. This is especially risky because the follow-up commit is specifically meant to prove propagation throughmain.py.Recommended fix shape: make the regression test exercise the actual
main.pybehavior rather than a copied helper. For example, extract the setup-exception discriminator into a small function imported by bothmain.pyand the test, or test the relevant entrypoint path with monkeypatched adapter setup raisingPrivilegedPluginInstallError. Once the test covers the real code path, I expect this to approve; I do not consider themain.pychange unnecessary scope.Fresh re-review on head
b95b0b0c: APPROVED.I reviewed the single follow-up commit from
7c315603. The main.py change is necessary: adapter_base can re-raise the privileged setup failure, but main.py had a broad outer setup exception handler that would otherwise downgrade it to a warning/reachable-but-misconfigured boot. Introducing PrivilegedPluginInstallError as a RuntimeError subclass and checking that exact type in main.py correctly lets privileged molecule-platform-mcp setup failures abort boot while preserving ordinary plugin behavior under the existing warning-and-continue path.Non-zero setup.sh and timeout paths now raise the dedicated subclass for the privileged plugin; ordinary plugins still record warnings/errors without aborting. CI statuses on
b95b0b0care all success. No blockers found.New commits pushed, approval review dismissed automatically according to repository settings
Fresh re-review on head
ca72b13e: APPROVED.I reviewed the follow-up commit from
b95b0b0c. The test concern is resolved: tests/test_main_privileged_plugin_failure.py now imports and exercises molecule_runtime.main._is_privileged_setup_failure instead of duplicating a test-local copy of the discriminator. The extracted helper preserves the approved production behavior: PrivilegedPluginInstallError still aborts the main.py setup path, while ordinary RuntimeError/ValueError cases remain non-privileged and continue through the existing warn-and-continue behavior.No adapter_base or builtins behavior regressed in this delta, and CI statuses on
ca72b13eare all success. No blockers found.5-axis re-review on new head
ca72b13e: APPROVED.Correctness/robustness: my prior REQUEST_CHANGES finding is resolved.
main.pynow extracts the real discriminator into_is_privileged_setup_failure(), and the outer setup exception handler calls that helper before deciding whether to re-raise.tests/test_main_privileged_plugin_failure.pyimports that exact production helper frommolecule_runtime.main, so the test no longer duplicates the rule in a test-local copy; ifmain.pydrifts away from this discriminator, the test target changes with the production code rather than silently validating stale duplicate logic.End-to-end behavior remains correct: privileged
molecule-platform-mcpsetup failures raisePrivilegedPluginInstallError, adapter-level install re-raises for the privileged plugin,main.pyidentifies that dedicated type and aborts boot loudly, and ordinary plugin/runtime exceptions still use the existing warning/degraded-boot path. Timeout and non-zero setup failure coverage remain present, and CI is green.Security/performance/readability: no remaining concerns. The extra
main.pyhelper is small, scoped, and justified by the regression-test need.