-
Notifications
You must be signed in to change notification settings - Fork 22
Collect clones and postpone triaging of y streams for CVEs #316
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?
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.
Code Review
This pull request introduces a new agent, clones_analyzer_agent.py, to identify and link cloned Jira issues. It also modifies the triage_agent.py to incorporate the new agent into its workflow, postponing triage of Y-stream CVEs until the corresponding Z-stream errata have been shipped. Additionally, the common/models.py file is updated to include schemas for the new agent, and the mcp_server/jira_tools.py file is modified to handle CVE triage eligibility. The Makefile and compose.yaml files are updated to include the new agent.
cf62623 to
85b1f4a
Compare
lbarcziova
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.
this will be amazing, thanks Maja!
| return CVEEligibilityResult( | ||
| is_cve=True, | ||
| is_eligible_for_triage=False, | ||
| when_eligible_for_triage=WhenEligibility.NEVER, |
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.
eventually, the embargo can be lifted, so this could be also later? But I don't remember for sure if Jotnar even sees those
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.
Searching for assignee = jotnar-project and "Embargo Status" = "True" gives no result. So probably we don't see them.
mcp_server/jira_tools.py
Outdated
| # @TODO: is this ok??? Or should I check something else? | ||
| return current_status.lower() == "closed" |
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.
this looks enough for me, but @opohorel @TomasKorbar or @ljavorsk can confirm
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def get_instructions() -> str: |
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.
how reliable is this when running locally? I am wondering if this wouldn't be more straightforward to do via tools using Jira API? Or it's not really straightforward?
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.
I would say it is reliable. I don't know if I can write a regexp for it. Because I don't know what to expect in the title. So probably this is safer. And in "this agent" there is still a missing step (one of the reason why this PR is still in draft) I should start suggesting if any clone is missing.
| To find other Jira issues which are clones of <JIRA_ISSUE> Jira issue, do the following: | ||
| 1. Search for other Jira issues which have the same affected component as <JIRA_ISSUE> Jira issue in RHEL Jira project and extract their titles. |
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.
Shall we use get_jira_details() here to get component name, issue title etc. to make the process more deterministic?
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.
It should be using it, since it is the passed tool for all the agent definitions. Or am I missing something?
85b1f4a to
ecdd2d7
Compare
ecdd2d7 to
13a369e
Compare
|
/gemini review |
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.
Code Review
This pull request introduces a new clones-analyzer-agent to identify cloned Jira issues and modifies the triage-agent to postpone the triage of Y-stream and maintenance-stream CVEs until their corresponding Z-stream clones are resolved. A new cron job is also added to periodically re-evaluate these postponed issues.
The changes are well-structured and address the goal of handling Y-stream CVEs more appropriately. However, I've found a critical logic error in how Z-stream issues are identified, which would prevent the postponement logic from working as intended. I've also identified several areas for code cleanup, including removing unused code and clarifying confusing logic. Please see my detailed comments for suggestions.
| if not re.match(r"^rhel-\d+\.\d+$", branch.lower()): | ||
| return True |
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.
There is a critical logic error here. The function check_z_stream_errata_shipped is intended to check the status of Z-stream issues, but the regex r"^rhel-\\d+\\.\\d+$" matches Y-streams (e.g., rhel-9.8), not Z-streams (e.g., rhel-9.8.z).
The current logic will check the status of Y-stream clones and incorrectly return True for Z-stream clones without checking their status. This defeats the purpose of postponing Y-stream triage.
The logic should be inverted to check for Z-streams. A simpler way to check for a Z-stream branch is to see if it ends with .z.
| if not re.match(r"^rhel-\d+\.\d+$", branch.lower()): | |
| return True | |
| if not branch.lower().endswith('.z'): | |
| return True |
| elif JiraLabels.POSTPONED.value in JiraLabels.all_labels(): | ||
| await tasks.set_jira_labels( | ||
| jira_issue=state.jira_issue, | ||
| labels_to_remove=[JiraLabels.POSTPONED.value], | ||
| dry_run=dry_run | ||
| ) |
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.
The condition JiraLabels.POSTPONED.value in JiraLabels.all_labels() is always true, which makes this elif block confusing. It will execute for any resolution other than POSTPONED, attempting to remove the POSTPONED label. However, all jotnar_ labels are already removed at the beginning of the workflow (line 629). This block is redundant, causes unnecessary API calls, and should be removed.
| def create_clones_analyzer_agent(mcp_tools: list[Tool], local_tool_options: dict[str, Any]) -> RequirementAgent: | ||
| return RequirementAgent(**get_agent_definition(mcp_tools)) |
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.
|
|
||
| setup_observability(os.environ["COLLECTOR_ENDPOINT"]) | ||
|
|
||
| dry_run = os.getenv("DRY_RUN", "False").lower() == "true" |
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.
| params={"fields": "status, assignee"}, | ||
| headers=headers, | ||
| ) as response: | ||
| response.raise_for_status() | ||
| current_issue = await response.json() | ||
| current_status = current_issue.get("fields", {}).get("status", {}).get("name", "") | ||
| current_assignee = current_issue.get("fields", {}).get("assignee", {}).get("name", "") |
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.
The current_assignee variable is fetched but never used. It should be removed, and the API call can be optimized to only fetch the status field.
| params={"fields": "status, assignee"}, | |
| headers=headers, | |
| ) as response: | |
| response.raise_for_status() | |
| current_issue = await response.json() | |
| current_status = current_issue.get("fields", {}).get("status", {}).get("name", "") | |
| current_assignee = current_issue.get("fields", {}).get("assignee", {}).get("name", "") | |
| params={"fields": "status"}, | |
| headers=headers, | |
| ) as response: | |
| response.raise_for_status() | |
| current_issue = await response.json() | |
| current_status = current_issue.get("fields", {}).get("status", {}).get("name", "") |
No description provided.