-
Notifications
You must be signed in to change notification settings - Fork 4
Add multiple personalities #15
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: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,3 +4,4 @@ | |
| **/__pycache__ | ||
| .vscode/** | ||
| findings.csv | ||
| apps/** | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| typos: | ||
| prompt: | | ||
| You are a code reviewer analyzing a single file's diff from a Pull Request. | ||
| Your task is to identify typos | ||
| Only report on typos. Return nothing if no typos found | ||
| priority: 5 | ||
| security: | ||
| prompt: | | ||
| You are a security reviewer analyzing a single file's diff from a Pull Request. | ||
| Only identify confirmed, high-confidence vulnerabilities introduced or modified in the diff. | ||
|
|
||
| # Strict rules: | ||
|
|
||
| Do not report vague or speculative issues like "potential path traversal" or "hardcoded secrets" unless they are clearly | ||
| exploitable and directly related to the categories above. | ||
|
|
||
| Do not report issues based only on pattern-matching or tool output—require code context and confirmation. | ||
| Retrieve the full file and other relevant files for context only after a suspicious change is detected in the diff. | ||
|
|
||
| A severity rating from 1 to 9 (9 is most critical) | ||
| Only report confirmed, context-aware vulnerabilities within the scope defined above | ||
| priority: 1 | ||
| codequality: | ||
| prompt: | | ||
| You are a code reviewer analyzing a single file's diff from a Pull Request. | ||
| Your task is to identify bad development patterns introduced or modified in the diff. | ||
| Focus only on poor coding practices that may lead to long-term maintainability, reliability, or readability issues. | ||
| Do not report security vulnerabilities or speculative risks. | ||
|
|
||
| Rules: | ||
| Only analyze changes in the diff. Ignore unchanged code or tool-generated output. | ||
| Retrieve the full file or other files for context only if needed to confirm the presence of a bad pattern. | ||
| Do not flag stylistic or formatting issues unless they reflect a deeper anti-pattern. | ||
| Examples of bad development patterns include: | ||
| Copy-pasted logic instead of reusable code | ||
| Excessive code nesting or deeply nested conditionals | ||
| Catch-all exception handling (e.g., catch(Exception) without handling) | ||
| Business logic in controllers or views | ||
| Logic dependent on hardcoded values where abstraction is expected | ||
| Functions or classes that are too long or do too much | ||
| Use of magic numbers or unclear naming | ||
|
|
||
| Only report confirmed, code-level development anti-patterns present in the diff. | ||
| priority: 3 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ | |
| from scm.adapters.git import GitAdapter | ||
| from util.git import parse_unified_diff | ||
| from util.filtering import should_process | ||
| from util.prompts import prompts | ||
| from util import prompts | ||
| from scm.adapters.github import Github | ||
| from scm import Scm | ||
| from shell import Shell | ||
|
|
@@ -31,7 +31,6 @@ | |
|
|
||
| from util.output import print_banner, write_csv | ||
|
|
||
| prompts = prompts() | ||
| load_dotenv(".env") | ||
|
|
||
| logger = logging.getLogger("saist") | ||
|
|
@@ -40,22 +39,24 @@ async def analyze_single_file(scm: Scm, adapter: BaseLlmAdapter, filename, patch | |
| """ | ||
| Analyzes a SINGLE file diff with OpenAI, returning a Findings object or None on error. | ||
| """ | ||
| system_prompt = prompts.DETECT | ||
| logger.debug(f"Processing {filename}") | ||
| prompt = ( | ||
| f"\n\nFile: {filename}\n{patch_text}\n" | ||
| ) | ||
| try: | ||
| return (await adapter.prompt_structured(system_prompt, prompt, Findings, [scm.read_file_contents])).findings | ||
| except Exception as e: | ||
| logger.error(f"[Error] File '{filename}': {e}") | ||
| return None | ||
| findings = [] | ||
| for analyst in prompts.analysts.keys(): | ||
| system_prompt = prompts.analysts[analyst].PROMPT | ||
| try: | ||
| findings += (await adapter.prompt_structured(system_prompt, prompt, Findings, [scm.read_file_contents])).findings | ||
| except Exception as e: | ||
| logger.error(f"[Error] File '{filename}': {e}") | ||
| return findings | ||
|
|
||
| def generate_summary_from_findings(adapter: BaseLlmAdapter, findings: list[Finding]) -> str: | ||
| """ | ||
| Uses OpenAI to generate a summary of all findings to be used as the PR review body. | ||
| """ | ||
| system_prompt = prompts.SUMMARY | ||
| system_prompt = prompts.summary_writer.PROMPT | ||
| for f in findings: | ||
| prompt = f"- **File**: `{f.file}`\n - **Issue**: {f.issue}\n - **Recommendation**: {f.recommendation}\n\n" | ||
|
|
||
|
|
@@ -212,8 +213,16 @@ async def main(): | |
|
|
||
| # Basic checks | ||
| if not file_name or not snippet or not issue: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Multiple debug logs for validation errors without clear context or actionable information. Priority: LOW CWE: N/A Recommendation: Consolidate debug logs into a single log statement with clear context or remove redundant logs. Snippet: |
||
| logging.debug("validation error for item") | ||
| item.line_number = -1 | ||
| continue | ||
| if "\n" in snippet: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Debug log for multi-line snippet without clear context or actionable information. Priority: LOW CWE: N/A Recommendation: Remove redundant debug logs or provide meaningful context for debugging. Snippet: |
||
| logging.debug("Code snippet contains multiple lines") | ||
| item.line_number = -1 | ||
| continue | ||
| if file_name not in file_line_maps: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Debug log for non-existent file without clear context or actionable information. Priority: LOW CWE: N/A Recommendation: Consolidate debug logs into a single log statement with clear context or remove redundant logs. Snippet: |
||
| logging.debug(f"{file_name} does not exist...") | ||
| item.line_number = -1 | ||
| # Possibly flagged a file that doesn't exist in the PR | ||
| continue | ||
|
|
||
|
|
@@ -228,6 +237,7 @@ async def main(): | |
| break | ||
|
|
||
| if not matched_new_line: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Debug log for unmatched snippet without clear context or actionable information. Priority: LOW CWE: N/A Recommendation: Remove redundant debug logs or provide meaningful context for debugging. Snippet: |
||
| logging.debug(f"Line '{snippet}' does not exist...") | ||
| # If we can't find the snippet in the patch, skip | ||
| item.line_number = -1 | ||
| continue | ||
|
|
@@ -252,8 +262,29 @@ async def main(): | |
| all_findings = list([x for x in all_findings if x.line_number != -1]) | ||
|
|
||
| if not all_findings: | ||
| print("No issues detected") | ||
| print("Followig validation, no valid issues detected") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Typo in the word 'Followig' Priority: LOW CWE: N/A Recommendation: Correct the typo to 'Following' Snippet: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Typo in the word 'Followig' Priority: LOW CWE: N/A Recommendation: Correct the typo to 'Following' Snippet: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Typo in print statement ('Followig' instead of 'Following'). Priority: LOW CWE: N/A Recommendation: Correct the typo in the print statement. Snippet: |
||
| exit(0) | ||
|
|
||
| print(f"✨ Validation complete! Identified {len(all_findings)} issues.\n") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Use of emoji in console output may not be suitable for all environments. Priority: LOW CWE: N/A Recommendation: Avoid using emojis in console output for better compatibility. Snippet: |
||
|
|
||
| # Deduplicate all_findings based on (file, line_number, cwe) | ||
|
|
||
| seen = set() | ||
| deduped_findings = [] | ||
|
|
||
| for finding in all_findings: | ||
| if finding.cwe == "N/A": | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Magic string comparison for 'N/A' which may lead to inconsistencies if the value changes or is reused elsewhere. Priority: LOW CWE: N/A Recommendation: Use a constant or enum for such values to ensure consistency and maintainability. Snippet: |
||
| deduped_findings.append(finding) | ||
| continue | ||
| key = (finding.file, finding.line_number, finding.cwe) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Deduplication logic is tightly coupled with the structure of Priority: LOW CWE: N/A Recommendation: Encapsulate the deduplication logic in a separate function or method to improve reusability and maintainability. Snippet: |
||
| if key not in seen: | ||
| seen.add(key) | ||
| deduped_findings.append(finding) | ||
|
|
||
| all_findings = deduped_findings | ||
|
|
||
| print(f"🚀 Deduplication complete! Identified {len(all_findings)} issues.\n") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Use of emoji in console output may not be suitable for all environments. Priority: LOW CWE: N/A Recommendation: Avoid using emojis in console output for better compatibility. Snippet: |
||
|
|
||
|
|
||
| if args.interactive: | ||
| s = Shell(llm, scm, all_findings) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,8 @@ | |
|
|
||
| class Finding(BaseModel): | ||
| file: str | ||
| snippet: Annotated[str, Field(description= "a single line code snipper containing the security issue") ] | ||
| category: str | ||
| snippet: Annotated[str, Field(description= "the single line of code snippet from the file most relevant to the detected issue") ] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Inconsistent field naming and description in the model. The field 'snippet' is annotated with a description that does not match the context of the model. Priority: LOW CWE: N/A Recommendation: Ensure the field description aligns with the model's purpose and naming conventions. Update the description to reflect the actual use case of the snippet field. Snippet: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Inconsistent field description format. The description for the 'snippet' field is overly verbose and could be simplified for clarity. Priority: LOW CWE: N/A Recommendation: Simplify the description to something like 'Relevant code snippet for the issue' to maintain consistency and readability. Snippet: |
||
| issue: str | ||
| recommendation: str | ||
| cwe: str | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,32 +1,61 @@ | ||
| class prompts(): | ||
| SUMMARY_PRE = """ | ||
| import os | ||
| import yaml | ||
|
|
||
| class personality(): | ||
| def __init__(self, prompt_body, prompt_suffix = None, priority = None): | ||
| self.prompt_body = prompt_body | ||
| self.prompt_suffix = prompt_suffix | ||
| if not priority: | ||
| priority = 1 | ||
| self.priority = priority | ||
|
|
||
| @property | ||
| def PROMPT(self): | ||
| return self.prompt_body + self.prompt_suffix | ||
|
|
||
| FILE_ANALYSIS_COMMON_SUFFIX = "Below is the diff for this single file. It starts with 'File: <filename>' followed by the unified diff.\n" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Hardcoded string used for a common suffix. This can lead to duplication and maintenance issues if the string needs to be updated in multiple places. Priority: LOW CWE: N/A Recommendation: Define this string as a constant in a configuration file or as a class-level constant to centralize its definition. Snippet: |
||
|
|
||
| summary_writer = personality( | ||
| prompt_body = """ | ||
| You are a senior application security engineer. | ||
| Given the following list of findings (issue descriptions and recommendations) | ||
| Write a concise but informative summary suitable for a GitHub Pull Request review comment. | ||
| It should be just a few sentences. | ||
| Group similar issues, and prioritize by severity. Use markdown formatting. | ||
| Return only the markdown summary, no other text. Do not put the markdown inside ``` | ||
| """ | ||
| SUMMARY_POST = """ | ||
| findings: | ||
| """ | ||
| DETECT_PRE = """ | ||
| You are a security reviewer analyzing a single file's diff from a Pull Request. | ||
| Look for issues in the OWASP top ten. Identify as many as you can. | ||
| Report multiple issues per line as seperate findings. | ||
| When you detect a vulnerability get the full file by retrieving its contents, use this for context. | ||
| You can also retrieve other files for context as needed. | ||
| Only report a vulnerability if exists in the original diff. | ||
| Do not report vulnerabilities that exist only in tool output | ||
| Provide a vulnerability priority between 1 and 9. 9 is most critical | ||
| Map each finding to a Common Weakness Enumeration ID (CWE). | ||
| """ | ||
| DETECT_POST = """" | ||
| Below is the diff for this single file. It starts with 'File: <filename>' followed by the unified diff.\n" | ||
| """ | ||
| @property | ||
| def SUMMARY(self): | ||
| return self.SUMMARY_PRE + self.SUMMARY_POST | ||
| @property | ||
| def DETECT(self): | ||
| return self.DETECT_PRE + self.DETECT_POST | ||
| """, | ||
| prompt_suffix = "findings:" | ||
| ) | ||
|
|
||
| def load_personalities(file_path='saist.personalities'): | ||
| if not os.path.exists(file_path): | ||
| raise FileNotFoundError(f"File '{file_path}' not found.") | ||
|
|
||
| with open(file_path, 'r') as file: | ||
| try: | ||
| personalities = yaml.safe_load(file) | ||
| if not isinstance(personalities, dict): | ||
| raise ValueError("YAML content is not a dictionary.") | ||
|
|
||
| for item_name, item_data in personalities.items(): | ||
| if not isinstance(item_data, dict): | ||
| raise ValueError(f"Item '{item_name}' must be a dictionary.") | ||
| if 'priority' not in item_data: | ||
| raise ValueError(f"Item '{item_name}' is missing required field: 'priority'") | ||
| if 'prompt' not in item_data: | ||
| raise ValueError(f"Item '{item_name}' is missing required field: 'prompt'") | ||
|
|
||
| return personalities | ||
|
|
||
| except yaml.YAMLError as e: | ||
| raise ValueError(f"Error parsing YAML file: {e}") | ||
|
|
||
| personalities_dict = load_personalities() | ||
|
|
||
| analysts = { | ||
| name: personality( | ||
| data["prompt"], | ||
| f"Set the Category to {name}. Set CWE to the format CWE-XXX or N/A if a CWE is not relevant" + | ||
| FILE_ANALYSIS_COMMON_SUFFIX, priority=data["priority"]) | ||
| for name, data in personalities_dict.items() | ||
| } | ||
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.
Security Issue: Hardcoded iteration over analysts' keys without error handling or validation.
Priority: LOW
CWE: N/A
Recommendation: Add validation for the existence of
prompts.analystsand handle cases where it might be empty or undefined.Snippet:
for analyst in prompts.analysts.keys():