-
Notifications
You must be signed in to change notification settings - Fork 1
add optional inclusion of the config file for analysis #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request adds an optional configuration file inclusion for AI analysis of support packets.
- Introduces a global variable and helper function to manage the sanitized configuration content.
- Extracts and reads sanitized_config.json when the include-config flag is enabled.
- Updates the CLI flags and integrates configuration content into analysis prompts.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| parser_support_packet.go | Adds global variable, helper to clear config content, and extraction logic. |
| main.go | Introduces the include-config flag and ensures previous config is cleared. |
| analyzer_llm.go | Appends configuration information into both the system and user prompts for analysis. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for optionally including the Mattermost configuration file (sanitized_config.json) in AI-driven log analysis.
- Introduces
SupportPacketResultto return both parsed logs and configuration content. - Extends
parseSupportPacketto extract and load the config file when the--include-configflag is set. - Propagates
configContentthroughprocessLogs,analyzeWithLLM, and all provider-specific functions; updates CLI flags, shell completions, and documentation.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| parser_support_packet.go | Changed parseSupportPacket signature to return *SupportPacketResult, added extraction and loading of sanitized_config.json. |
| main.go | Added includeConfig flag, updated calls to processLogs to pass configContent, and updated flag registration/completion. |
| analyzer_llm.go | Extended analyzeWithLLM, prepareAnalysisPrompts, and provider wrappers to accept and incorporate configContent. |
| README.md | Documented the --include-config flag, updated examples and descriptions. |
Comments suppressed due to low confidence (3)
parser_support_packet.go:9
- The import block currently only includes
"strings", but this file useszip.OpenReader,fmt.Errorf,os.MkdirTemp,filepath.Join, andlogger.*. Add missing imports for"archive/zip","fmt","os","path/filepath", and the logger package.
import (
main.go:301
- [nitpick] Consider adding unit tests for the
--include-configflag andSupportPacketResultlogic to verify thatsanitized_config.jsonis correctly extracted, loaded intoConfigContent, and passed through to the AI analysis pipeline.
cmd.Flags().BoolVar(&includeConfig, "include-config", false, "Include configuration in AI analysis (support-packet only)")
parser_support_packet.go:41
- Variables
aiAnalyzeandincludeConfigare referenced here but not passed intoparseSupportPacket. Either inject these flags as parameters or ensure they are available as package-level variables to avoid undefined identifier errors.
if aiAnalyze && includeConfig {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds optional inclusion of the Mattermost configuration file (sanitized_config.json) in AI analysis of support packets.
- parseSupportPacket now returns both logs and configuration content via
SupportPacketResult - CLI updates: added
--include-configflag and extendedprocessLogsto forward config to LLM calls - Analyzer enhancements: all LLM provider functions and prompt preparation now include config data when present
- Documentation updated to cover the new flag and behavior
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| parser_support_packet.go | Return *SupportPacketResult, extract and include sanitized_config.json when requested |
| main.go | Added includeConfig flag, updated processLogs signature and support-packet command |
| analyzer_llm.go | Extended analyzeWithLLM and provider methods to accept and incorporate configContent |
| README.md | Documented --include-config flag and updated support-packet usage examples |
Comments suppressed due to low confidence (1)
parser_support_packet.go:18
- Update this comment to mention that the function now returns
*SupportPacketResultand optionally includes the sanitized configuration content.
// parseSupportPacket extracts and parses logs from a Mattermost support packet zip file
| // Extract sanitized_config.json if needed for AI analysis | ||
| var configPath string | ||
| if aiAnalyze && includeConfig { | ||
| for _, file := range reader.File { | ||
| if strings.HasSuffix(file.Name, "sanitized_config.json") { | ||
| configPath = filepath.Join(tempDir, "sanitized_config.json") | ||
| if err := extractZipFile(file, configPath); err != nil { | ||
| logger.Warn("Failed to extract sanitized_config.json from support packet", "file", file.Name, "error", err) | ||
| configPath = "" // Reset if extraction failed | ||
| } else { | ||
| logger.Debug("Extracted sanitized_config.json for AI analysis", "path", configPath) | ||
| } | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Look for log files in the zip | ||
| for _, file := range reader.File { |
Copilot
AI
May 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The code iterates over reader.File twice (once for config and once for log files). Consider merging these passes into a single loop to reduce redundant iteration and simplify control flow.
| // Extract sanitized_config.json if needed for AI analysis | |
| var configPath string | |
| if aiAnalyze && includeConfig { | |
| for _, file := range reader.File { | |
| if strings.HasSuffix(file.Name, "sanitized_config.json") { | |
| configPath = filepath.Join(tempDir, "sanitized_config.json") | |
| if err := extractZipFile(file, configPath); err != nil { | |
| logger.Warn("Failed to extract sanitized_config.json from support packet", "file", file.Name, "error", err) | |
| configPath = "" // Reset if extraction failed | |
| } else { | |
| logger.Debug("Extracted sanitized_config.json for AI analysis", "path", configPath) | |
| } | |
| break | |
| } | |
| } | |
| } | |
| // Look for log files in the zip | |
| for _, file := range reader.File { | |
| // Process files in the zip | |
| var configPath string | |
| for _, file := range reader.File { | |
| // Check if it's the sanitized_config.json file | |
| if aiAnalyze && includeConfig && strings.HasSuffix(file.Name, "sanitized_config.json") { | |
| configPath = filepath.Join(tempDir, "sanitized_config.json") | |
| if err := extractZipFile(file, configPath); err != nil { | |
| logger.Warn("Failed to extract sanitized_config.json from support packet", "file", file.Name, "error", err) | |
| configPath = "" // Reset if extraction failed | |
| } else { | |
| logger.Debug("Extracted sanitized_config.json for AI analysis", "path", configPath) | |
| } | |
| continue | |
| } |
|
|
||
| // analyzeWithLLM routes the log analysis to the appropriate LLM provider | ||
| func analyzeWithLLM(logs []LogEntry, config LLMConfig) error { | ||
| func analyzeWithLLM(logs []LogEntry, config LLMConfig, configContent string) error { |
Copilot
AI
May 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] To simplify function signatures and reduce duplication, consider adding ConfigContent as a field on LLMConfig instead of passing it as a separate parameter through all analyzeWith* functions.
| func analyzeWithLLM(logs []LogEntry, config LLMConfig, configContent string) error { | |
| func analyzeWithLLM(logs []LogEntry, config LLMConfig) error { |
|
@svelle Is this ready for review? |
Yes indeed |
Co-authored-by: Copilot <[email protected]>
hanzei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat idea 👍
| return result, nil | ||
| } | ||
|
|
||
| // extractZipFile extracts a single file from a zip archive to the specified path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this in memory instead of writing the file to disk? Seems like a potential security issue.
| // SupportPacketResult contains the parsed logs and optional configuration content | ||
| type SupportPacketResult struct { | ||
| Logs []LogEntry | ||
| ConfigContent string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return the content instead of a path? That makes it easier to the caller.
No description provided.