feat(ai): Let the assistant edit files, run actions, and control deployments#185
Conversation
Code Review SummaryThis PR adds several powerful tools to the AI assistant, enabling it to manage files, execute quick actions, control deployment states, and retrieve security events. The implementation correctly handles authorization and 'fail-closed' security checks. 🚀 Key Improvements
💡 Minor Suggestions
|
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
| if !ok { | ||
| return "", fmt.Errorf("content is required") | ||
| } | ||
| if blocked, reason, perr := s.protectedDeploymentActionBlocked(name, protectedActionUploadFile); perr == nil && blocked { |
There was a problem hiding this comment.
The current logic 'fails open' if an error occurs while checking if the action is blocked (e.g., a database connection issue or configuration error). It is safer to fail closed by explicitly handling the error returned by protectedDeploymentActionBlocked to ensure the security check is enforced.
| if blocked, reason, perr := s.protectedDeploymentActionBlocked(name, protectedActionUploadFile); perr == nil && blocked { | |
| blocked, reason, perr := s.protectedDeploymentActionBlocked(name, protectedActionUploadFile) | |
| if perr != nil { | |
| return "", perr | |
| } | |
| if blocked { | |
| return "", fmt.Errorf("%s", reason) | |
| } |
| if actionID == "" { | ||
| return "", fmt.Errorf("action_id is required") | ||
| } | ||
| if blocked, reason, perr := s.protectedDeploymentActionBlocked(name, protectedActionQuickAction); perr == nil && blocked { |
There was a problem hiding this comment.
Similar to the file upload tool, the protection check here should handle potential errors from the check itself. If perr is non-nil, the code currently bypasses the block check.
| if blocked, reason, perr := s.protectedDeploymentActionBlocked(name, protectedActionQuickAction); perr == nil && blocked { | |
| blocked, reason, perr := s.protectedDeploymentActionBlocked(name, protectedActionQuickAction) | |
| if perr != nil { | |
| return "", perr | |
| } | |
| if blocked { | |
| return "", fmt.Errorf("%s", reason) | |
| } |
822eb52 to
2146649
Compare
…oyments The assistant's tools were read-only. It can now write a file in a deployment's directory, run a configured quick action, start, stop, or restart a deployment, and list a deployment's recent security events to summarize them. Every state-changing tool requires write access to the deployment and honours protected mode, so the assistant can only do what the operator behind it is allowed to do. File writes stay inside the deployment directory.
2146649 to
941a138
Compare
| default: | ||
| return "", fmt.Errorf("action must be start, stop, or restart") | ||
| } | ||
| if err != nil { |
There was a problem hiding this comment.
Output from deployment control operations (like start, stop, or restart) often includes logs from the orchestration engine or the container's standard output. These logs can contain sensitive information such as environment variables, database connection strings, or internal system paths. Consistent with the run_quick_action tool, this output should be passed through the redactor to ensure no secrets are exposed to the AI model or the session logs.
| if err != nil { | |
| if err != nil { | |
| return "", err | |
| } | |
| redactor := ai.NewRedactor(s.deploymentSecretValues(name)) | |
| redacted, _ := redactor.Redact(output) | |
| return truncateToolOutput(fmt.Sprintf("Ran %s on deployment %s.\n%s", action, name, redacted)), nil |
| if s.securityManager == nil { | ||
| return "The security module is not enabled.", nil | ||
| } | ||
| limit := 50 |
There was a problem hiding this comment.
While a default limit is provided, there is no upper bound enforced on the limit parameter. Allowing the AI to request an unlimited number of security events could lead to significant memory allocations or timeouts if a deployment has a large event history. It is safer to cap the maximum number of events returned (e.g., to 100).
| limit := 50 | |
| limit := 50 | |
| if v, ok := args["limit"].(float64); ok && v > 0 { | |
| limit = int(v) | |
| if limit > 100 { | |
| limit = 100 | |
| } | |
| } |
| fmt.Fprintf(&b, "- %s [%s] %s %s %d from %s: %s\n", | ||
| e.CreatedAt.Format(time.RFC3339), e.Severity, e.RequestMethod, e.RequestPath, e.StatusCode, e.SourceIP, e.Message) | ||
| } | ||
| return truncateToolOutput(b.String()), nil |
There was a problem hiding this comment.
Security event logs can often contain sensitive data such as session tokens, API keys, or personal information within request paths or log messages. To prevent accidental exposure of these secrets to the AI model or session logs, the output should be processed through the redactor using the deployment's secret values, consistent with how the run_quick_action tool handles output.
| return truncateToolOutput(b.String()), nil | |
| redactor := ai.NewRedactor(s.deploymentSecretValues(name)) | |
| redacted, _ := redactor.Redact(b.String()) | |
| return truncateToolOutput(redacted), nil |
1 participant
First set of #147. The assistant's built-in tools were read-only; this adds four, with the framework kept read-only-by-default and each state-changing tool authorizing write access itself:
write_deployment_filewrites a file in a deployment's directory (path confined to it, honours protected mode's upload gate).run_quick_actionruns a configured quick action by id (honours the quick-action protected gate).control_deploymentstarts, stops, or restarts a whole deployment.get_security_eventslists a deployment's recent security events to summarize (read-only, and returns a clear message when the security module is off).The three mutating tools require deployment write access, verified by a test that a viewer is refused; file writes are confined to the deployment directory, verified by a traversal test. These become part of the shared tool set that #184 will also expose over MCP.