Harden MCP runtime with operator policy and integration coverage#14
Conversation
|
Review guide for first pass:
Main follow-ups intentionally left for later:
|
There was a problem hiding this comment.
💡 Codex Review
master-control/src/master_control/core/runtime.py
Lines 728 to 730 in a2f51c9
After a confirmed call claims a pending approval, the invoke path only handles ToolError. If tool.invoke(...) raises any other exception (for example PermissionError/OSError from filesystem operations), execution exits before _finalize_claimed_tool_approval(...) runs, leaving that approval stuck in executing and no longer rejectable/approvable through normal flows. This wedges approval state and can require manual DB cleanup; finalize the claimed approval in a broader exception/finally path.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| AND status = 'pending' | ||
| AND arguments_json = ? | ||
| AND audit_context_json = ? | ||
| ORDER BY id DESC |
There was a problem hiding this comment.
Claim pending approvals by id, not full audit_context
Approval claiming requires exact audit_context_json equality in addition to tool and arguments, so confirmations executed from a different interface context (e.g., pending created with source="mcp_stdio" but confirmed via CLI/chat command) will run the tool without claiming the original pending row. That leaves stale pending approvals that can later be approved and replay the same mutation unexpectedly. Matching by approval id (or normalizing context for claim) avoids this orphan/replay behavior.
Useful? React with 👍 / 👎.
Summary
This change hardens the MCP-first runtime baseline in four areas:
mc doctormc mcp-serveWhat Changed
Runtime and policy
src/master_control/policy/config.pyPolicyEngineto enforce tool enablement, confirmation overrides, allowed scopes, and service patternsMasterControlRuntime.doctor()ConfigManagerconsume managed targets from policy instead of a parallel hardcoded sourceInterface and ownership cleanup
core.runtime -> interfaces.agent.*coupling by moving runtime imports tomaster_control.agent.*MasterControlApp.doctor()to delegate to the runtimeIntegration coverage
tests/test_runtime_policy_integration.pytests/test_mcp_stdio_integration.pyDocs
docs/policy.mddocs/runtime-integration-testing.mddocs/history/Review Guide
src/master_control/policy/config.pyandsrc/master_control/policy/engine.py.src/master_control/core/runtime.py, especiallydoctor()andrun_tool()policy application.tests/test_runtime_policy_integration.pyandtests/test_mcp_stdio_integration.py..github/workflows/ci.yml,docs/policy.md, anddocs/runtime-integration-testing.md.Validation
python3 -m ruff check .python3 -m mypy srcPYTHONPATH=src python3 -m unittest discover -s testsPYTHONPATH=src python3 -m pytest -q tests --ignore tests/test_runtime_policy_integration.py --ignore tests/test_mcp_stdio_integration.pyPYTHONPATH=src python3 -m pytest -q tests/test_runtime_policy_integration.py tests/test_mcp_stdio_integration.pypython3 -m compileall srcPYTHONPATH=src python3 -m master_control --json doctorpython3 scripts/validate_operator_bootstrap.py --output-dir /tmp/mc-bootstrap-analysis-final --provider heuristic --python python3Residual Risks
systemdscenariosFollow-up Checklist