[agent] Optimize the agent structure by moving the confirmation logic and risk level assessment down to the security service.

[CLFutureX]

Motivation:
Right now, the agent has extra logic mixed in. The agent should only handle coordination and scheduling—extra logic should go to the right areas it belongs to.
Modification:
Move the current requires_confirmation and extract_security_risk logic down to the security area, and let the security service manage them.

[CLFutureX]

@malhotra5 @xingyaoww @enyst hey，PTAL，thanks

[malhotra5]

I'm concerned about including a security service class in the base agent class

some of the security related logic is very specific to the default agent class Agent that we provide

other agent implementations may not require similar logic (extract_security_risk method is the primary example of this)

[malhotra5]

The original method handled both decision-making and state mutation, but now they're split across classes. This split could lead to inconsistencies if the agent forgets to set the execution status, or if the logic gets out of sync. Any ideas on how we may address this?

[CLFutureX]

Oh, the reason for this adjustment is to avoid modifying external resources. Instead, we leave the right to make modifications to the caller, preserving the read-only nature of this method.
If needed, I think we can implement a method in DefaultSecurityService to handle the state.

[malhotra5]

I think splitting the security context from the agent is the right direction! However, the security context can affect the agent's control loop so I think we want an interface that

1. is flexible enough so different agents can implement their own requirements (based on the analyzer outputs, security policy, state, etc)
2. returns a result object that includes both the decision and the required state changes, so the agent can coordinate reliably based on the results

[CLFutureX]

I think splitting the security context from the agent is the right direction! However, the security context can affect the agent's control loop so I think we want an interface that

1. is flexible enough so different agents can implement their own requirements (based on the analyzer outputs, security policy, state, etc)
2. returns a result object that includes both the decision and the required state changes, so the agent can coordinate reliably based on the results

Thanks for your suggestions! Based on your input, I've made the following adjustments:
Designed a three-layer structure: Top-level Interface → Base Class → Concrete Implementation
1.1 Top-level Interface: Only defines common methods to facilitate future extensions.
1.2 Base Class: Implements the universal method requires_confirmation while introducing the shared resource state, avoiding repetitive implementation in subclasses.
1.3 Final Implementation Class: Focuses on unique implementations, such as the existing method extract_security_risk.
Moved the security analysis class down from AgentBase to Agent.

[malhotra5]

Left some comments, I think we're getting closer but would like to be a bit more intentional regarding the interfaces and how we enforce separation of concerns between the agent and the security related logic

[malhotra5]

SecurityService (interface)
├── SecurityServiceBase (base implementation)
    └── DefaultSecurityService (concrete implementation)

We seem to have multiple layers for the interface, could we reduce them?

[CLFutureX]

Got it, adjustments done!

[malhotra5]

Suggested change

	def __init__(self, state: "ConversationState"):
	def __init__(
	self,
	security_analyzer: SecurityAnalyzerBase | None,
	confirmation_policy: ConfirmationPolicy
	):

Let's de-couple the security service as much as possible. Ideally we don't pass the entire state object. The security service should return a typed object which includes the final decision (whether the control loop should pause, reject, etc) and the agent can use to modify its state properly

Also I'm considering returning a typed object with the final security assessment, so that the agent can interpret it easily and we can standardize the assessment requirements. for example, the method requires_confirmation returns the following typed object

class SecurityAssessment:
    requires_confirmation: bool
    overall_risk_assessment: SecurityRisk

[CLFutureX]

Let's de-couple the security service as much as possible. Ideally we don't pass the entire state object. The security service should return a typed object which includes the final decision (whether the control loop should pause, reject, etc) and the agent can use to modify its state properly

Also I'm considering returning a typed object with the final security assessment, so that the agent can interpret it easily and we can standardize the assessment requirements. for example, the method requires_confirmation returns the following typed object

class SecurityAssessment:
    requires_confirmation: bool
    overall_risk_assessment: SecurityRisk

For now, I still choose to keep ConversationState, since both confirmation_policy and security_analyzer can be updated during the conversation.

[malhotra5]

Suggested change

	def requires_confirmation(
	def assess_actions(

I think maybe we should make this a more generic name. Possibly even pass the entire event history here as well since we may develop other security policies based on prior events

[CLFutureX]

Got it, Adjusted to access_confirm.

[CLFutureX]

Left some comments, I think we're getting closer but would like to be a bit more intentional regarding the interfaces and how we enforce separation of concerns between the agent and the security related logic

Hey, Thanks for your suggestions!
based on your suggestions, I've made the following adjustments:
1 Defined security_service as a Pydantic field and turned it into a public service.(To avoid abstracting the highly personalized extract_security_risk method into an interface, I chose to directly set its type as DefaultSecurityService. WDYT?)
2 Removed unnecessary intermediate classes.
3 Optimized method names while keeping ConversationState as a parameter—since both confirmation_policy and security_analyzer can be updated during the conversation.
4 Adjusted the return object: it now includes a "confirmation required" flag + risk level (defaults to the highest risk level among all actions).

PTAL， thanks

[malhotra5]

Thanks for the changes!

3 Optimized method names while keeping ConversationState as a parameter—since both confirmation_policy and security_analyzer can be updated during the conversation.

I'd still prefer if this wasn't the case. The reason being that the control loop is stopped by the agent and reflected by modifying state parameters.

So we should avoid manipulating the state object outside of the agent. While that's not the case today, passing the entire state object can imply that its allowed.

The separation I'd like is

1. agent - orchestrates the control loop, modifies its state as needed to reflect its current status
2. security service - looks at events, passes a decision to the agent to halt agent loop

I think if we pass a reference from the state object state.confirmation_policy, and its value is changed later, the updated value would be reflected in the security service as well? Maybe we could write a unit test to confirm whether that's the case? If not then there may be other workaround that I'm happy to ideate. LMK what you think!