Migrate agents to Semantic Kernel agent framework and add kernel approval filter#13
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| self.termination_strategy = default_termination_strategy.DefaultTerminationStrategy( | ||
| maximum_iterations=10 | ||
| ) | ||
| logger.info("OrchestratorSK initialized with Semantic Kernel AgentGroupChat") |
There was a problem hiding this comment.
Define logger before use in OrchestratorSK
The constructor calls logger.info(...) but this module no longer defines or imports logger, so initializing OrchestratorSK will raise a NameError in any runtime path that instantiates it. This breaks all chat routing on startup; add the missing get_logger import and logger = get_logger(__name__) (or remove the log call) to restore initialization.
Useful? React with 👍 / 👎.
| function_name = context.function.name | ||
| plugin_name = context.function.plugin_name or "" | ||
|
|
||
| if plugin_name != "execution": | ||
| await next(context) |
There was a problem hiding this comment.
Approval filter may never run for execution tools
The filter returns early unless context.function.plugin_name == "execution", but execution plugins are now registered via ChatCompletionAgent(..., plugins=plugins) without an explicit plugin name. In Semantic Kernel, that typically yields a plugin name derived from the class (e.g., ExecutionPlugin), so the check will skip run_readonly_command/run_test_by_id and the new approval gate won’t apply. If the intent is to enforce approvals on execution calls, either register the plugin under the execution name or relax this check to match the actual plugin name.
Useful? React with 👍 / 👎.
Motivation
ChatCompletionAgent,AgentGroupChat) to improve reliability and routing.Description
SAPAutomationAgent(subclassingsemantic_kernel.agents.ChatCompletionAgent) insrc/agents/agents/base.pyand exported it via the agents package.AgentGroupChatwith a kernel-based selection function and termination strategy insrc/agents/agents/orchestrator.py; removed the manualhandle_chatloop and fragile JSON routing extraction.EchoAgentSK,SystemContextAgentSK,TestAdvisorAgentSK,ActionPlannerAgentSK,ActionExecutorAgent) to inherit fromSAPAutomationAgent, register their plugins via the agentpluginsparameter, and drop manual loop/registration boilerplate.src/agents/filters/approval_filter.pyand registered it insrc/agents/sk_kernel.pyto validateexecution.*function calls (e.g.,run_readonly_command,run_test_by_id) before invocation; addedsrc/agents/filters/__init__.py.ACTION_EXECUTOR_SYSTEM_PROMPTand removed embedded orchestrator routing instructions) insrc/agents/prompts.pyso agents only declare their responsibilities.AgentRegistry.all_agents()helper and updated default registry creation insrc/agents/agents/base.pyto wire plugins and instantiate agents using the new agent classes.Testing
create_kernel()configured) before deployment to validate behavioral and safety changes.Codex Task