diff --git a/AUTO1_PATCHES.md b/AUTO1_PATCHES.md index 4e11613b6d..8e603d7fcd 100644 --- a/AUTO1_PATCHES.md +++ b/AUTO1_PATCHES.md @@ -18,7 +18,7 @@ Keep this list minimal to ease upstream rebases. | review-severity-emoji | OPS-24090 | Enhancement | pr_agent/algo/utils.py | Add a header emoji for findings severity summary in PR review output. | Not upstreamed yet. | Remove once upstream adds a default emoji for findings severity summary. | | improve-reflect-fallback-score | OPS-24090 | Fix | pr_agent/tools/pr_code_suggestions.py | Prevent inline suggestion spam when self-reflection fails by defaulting all suggestion scores to 0. | Not upstreamed yet. | Remove once upstream handles reflect failure without inflating scores. | | describe-mermaid-sanitize | OPS-24090 | Fix | pr_agent/tools/pr_description.py | Strip backticks from Mermaid diagrams to avoid GitHub render failures. | Not upstreamed yet. | Remove once upstream sanitizes Mermaid diagrams or fixes prompt output. | -| jira-cloud-ticket-context | OPS-24091 | Enhancement | pr_agent/algo/utils.py, pr_agent/tickets/jira_cloud.py, pr_agent/tools/ticket_pr_compliance_check.py, pr_agent/tools/pr_reviewer.py, pr_agent/tools/pr_description.py, pr_agent/tools/pr_config.py, tests/unittest/test_convert_to_markdown.py, tests/unittest/test_jira_cloud_ticket_context.py, tests/unittest/test_pr_reviewer_ticket_note.py, tests/unittest/test_ticket_pr_compliance_check.py | Fetch Jira Cloud ticket context using PR title first and branch name only as fallback, support both basic auth and scoped-token Jira Cloud API access, ignore placeholder compliance bullets, and show review notes when the Jira ticket cannot be fetched. | Not upstreamed yet. | Remove once upstream supports Jira Cloud ticket context from PR metadata with title-first precedence, scoped-token auth, placeholder-safe compliance rendering, and comment-level fetch-failure notes. | +| jira-cloud-ticket-context | OPS-24091 | Enhancement | pr_agent/algo/utils.py, pr_agent/tickets/jira_cloud.py, pr_agent/tools/ticket_pr_compliance_check.py, pr_agent/tools/pr_reviewer.py, pr_agent/tools/pr_description.py, pr_agent/tools/pr_config.py, tests/unittest/test_convert_to_markdown.py, tests/unittest/test_jira_cloud_ticket_context.py, tests/unittest/test_pr_reviewer_ticket_note.py, tests/unittest/test_ticket_pr_compliance_check.py | Fetch Jira Cloud ticket context using PR title first and branch name only as fallback, support both basic auth and scoped-token Jira Cloud API access, ignore placeholder compliance bullets and placeholder text like `None.`, show review notes when the Jira ticket cannot be fetched, and normalize empty discovered tickets before prompt rendering. | Not upstreamed yet. | Remove once upstream supports Jira Cloud ticket context from PR metadata with title-first precedence, scoped-token auth, placeholder-safe compliance rendering, comment-level fetch-failure notes, and prompt-safe normalization for empty ticket payloads. | ## Rebase checklist diff --git a/pr_agent/algo/utils.py b/pr_agent/algo/utils.py index 7739866171..49411a2d0a 100644 --- a/pr_agent/algo/utils.py +++ b/pr_agent/algo/utils.py @@ -379,18 +379,38 @@ def extract_relevant_lines_str(end_line, files, relevant_file, start_line, deden return "" -def normalize_ticket_requirement_text(text: str) -> str: - text = (text or '').strip() +_PLACEHOLDER_REQUIREMENT_ITEMS = frozenset({ + "", "-", "*", "none", "(none)", "n/a", "(n/a)", "na", "(na)", + "no information", "no information provided", "none provided", "not applicable", +}) +_REQUIREMENT_ITEM_PREFIX_RE = re.compile(r"^(?:[-*•]+|\d+[.)]|\[[ xX]\])\s*") + + +def _canonicalize_requirement_item(line: str) -> str: + canonical_line = (line or "").strip() + while canonical_line: + updated_line = _REQUIREMENT_ITEM_PREFIX_RE.sub("", canonical_line, count=1).strip() + if updated_line == canonical_line: + break + canonical_line = updated_line + return canonical_line.lower().rstrip(".:;!,") + + +def is_placeholder_requirement_item(line: str) -> bool: + return _canonicalize_requirement_item(line) in _PLACEHOLDER_REQUIREMENT_ITEMS + + +def parse_requirement_items(text: str) -> list[str]: + text = (text or "").strip() if not text: - return '' + return [] - normalized_lines = [line.strip() for line in text.splitlines() if line.strip()] - placeholder_lines = {'-', '*', 'none', '(none)', 'n/a', '(n/a)'} - normalized_lines = [line for line in normalized_lines if line.lower() not in placeholder_lines] - if not normalized_lines: - return '' + requirement_items = [line.strip() for line in text.splitlines() if line.strip()] + return [item for item in requirement_items if not is_placeholder_requirement_item(item)] - return '\n'.join(normalized_lines) + +def normalize_ticket_requirement_text(text: str) -> str: + return '\n'.join(parse_requirement_items(text)) def ticket_markdown_logic(emoji, markdown_text, value, gfm_supported, ticket_note: str = '') -> str: @@ -408,27 +428,30 @@ def ticket_markdown_logic(emoji, markdown_text, value, gfm_supported, ticket_not ticket_url = ticket_analysis.get('ticket_url', '').strip() explanation = '' ticket_compliance_level = '' # Individual ticket compliance - fully_compliant_str = normalize_ticket_requirement_text(ticket_analysis.get('fully_compliant_requirements')) - not_compliant_str = normalize_ticket_requirement_text(ticket_analysis.get('not_compliant_requirements')) - requires_further_human_verification = normalize_ticket_requirement_text( + fully_compliant_items = parse_requirement_items(ticket_analysis.get('fully_compliant_requirements')) + not_compliant_items = parse_requirement_items(ticket_analysis.get('not_compliant_requirements')) + requires_further_human_verification_items = parse_requirement_items( ticket_analysis.get('requires_further_human_verification') ) + fully_compliant_str = '\n'.join(fully_compliant_items) + not_compliant_str = '\n'.join(not_compliant_items) + requires_further_human_verification = '\n'.join(requires_further_human_verification_items) - if not fully_compliant_str and not not_compliant_str: + if not fully_compliant_items and not not_compliant_items: get_logger().debug(f"Ticket compliance has no requirements", artifact={'ticket_url': ticket_url}) continue # Calculate individual ticket compliance level - if fully_compliant_str: - if not_compliant_str: + if fully_compliant_items: + if not_compliant_items: ticket_compliance_level = 'Partially compliant' else: - if not requires_further_human_verification: + if not requires_further_human_verification_items: ticket_compliance_level = 'Fully compliant' else: ticket_compliance_level = 'PR Code Verified' - elif not_compliant_str: + elif not_compliant_items: ticket_compliance_level = 'Not compliant' # Store the compliance level for aggregation @@ -436,20 +459,20 @@ def ticket_markdown_logic(emoji, markdown_text, value, gfm_supported, ticket_not all_compliance_levels.append(ticket_compliance_level) # build compliance string - if fully_compliant_str: + if fully_compliant_items: explanation += f"Compliant requirements:\n\n{fully_compliant_str}\n\n" - if not_compliant_str: + if not_compliant_items: explanation += f"Non-compliant requirements:\n\n{not_compliant_str}\n\n" - if requires_further_human_verification: + if requires_further_human_verification_items: explanation += f"Requires further human verification:\n\n{requires_further_human_verification}\n\n" ticket_compliance_str += f"\n\n**[{ticket_url.split('/')[-1]}]({ticket_url}) - {ticket_compliance_level}**\n\n{explanation}\n\n" # for debugging - if requires_further_human_verification: + if requires_further_human_verification_items: get_logger().debug(f"Ticket compliance requires further human verification", artifact={'ticket_url': ticket_url, - 'requires_further_human_verification': requires_further_human_verification, - 'compliance_level': ticket_compliance_level}) + 'requires_further_human_verification': requires_further_human_verification, + 'compliance_level': ticket_compliance_level}) except Exception as e: get_logger().exception(f"Failed to process ticket compliance: {e}") diff --git a/pr_agent/tickets/jira_cloud.py b/pr_agent/tickets/jira_cloud.py index 21408bf999..89ebfcadd8 100644 --- a/pr_agent/tickets/jira_cloud.py +++ b/pr_agent/tickets/jira_cloud.py @@ -442,6 +442,7 @@ def _extract_subtasks_as_sub_issues(issue: Dict[str, Any], base_url: str, limit: "ticket_url": _jira_issue_url(base_url, key), "title": summary or key, "body": "", + "labels": "", } ) diff --git a/pr_agent/tools/pr_reviewer.py b/pr_agent/tools/pr_reviewer.py index 3d97ba2f3c..f4cb563370 100644 --- a/pr_agent/tools/pr_reviewer.py +++ b/pr_agent/tools/pr_reviewer.py @@ -15,7 +15,7 @@ from pr_agent.algo.token_handler import TokenHandler from pr_agent.algo.utils import (ModelType, PRReviewHeader, convert_to_markdown_v2, github_action_output, - load_yaml, normalize_ticket_requirement_text, + load_yaml, parse_requirement_items, show_relevant_configurations) from pr_agent.config_loader import get_settings from pr_agent.git_providers import (get_git_provider, @@ -46,11 +46,11 @@ def build_suspected_ticket_mismatch_note(ticket_compliance_check) -> str: if not isinstance(ticket_analysis, dict): continue - fully_compliant_str = normalize_ticket_requirement_text(ticket_analysis.get("fully_compliant_requirements")) - not_compliant_str = normalize_ticket_requirement_text(ticket_analysis.get("not_compliant_requirements")) - if fully_compliant_str: + fully_compliant_items = parse_requirement_items(ticket_analysis.get("fully_compliant_requirements")) + not_compliant_items = parse_requirement_items(ticket_analysis.get("not_compliant_requirements")) + if fully_compliant_items: return "" - if not_compliant_str: + if not_compliant_items: has_not_compliant_requirements = True if not has_not_compliant_requirements: diff --git a/pr_agent/tools/ticket_pr_compliance_check.py b/pr_agent/tools/ticket_pr_compliance_check.py index adbc9e4cca..2e697b0ff0 100644 --- a/pr_agent/tools/ticket_pr_compliance_check.py +++ b/pr_agent/tools/ticket_pr_compliance_check.py @@ -12,6 +12,58 @@ r'(https://github[^/]+/[^/]+/[^/]+/issues/\d+)|(\b(\w+)/(\w+)#(\d+)\b)|(#\d+)' ) +TICKET_PROMPT_FIELDS = ("ticket_id", "ticket_url", "title", "body", "labels", "requirements") + + +def _normalize_ticket_value(value): + if value is None: + return "" + if isinstance(value, str): + return value + if isinstance(value, (list, tuple, set)): + return ", ".join(str(item) for item in value if item is not None) + return str(value) + + +def _normalize_ticket_for_prompt(ticket): + ticket = ticket if isinstance(ticket, dict) else {} + normalized_ticket = { + field: _normalize_ticket_value(ticket.get(field, "")) for field in TICKET_PROMPT_FIELDS + } + + sub_issues = ticket.get("sub_issues") + if isinstance(sub_issues, list): + normalized_sub_issues = [] + for sub_issue in sub_issues: + normalized_sub_issue = _normalize_ticket_for_prompt(sub_issue) + if _ticket_has_prompt_content(normalized_sub_issue): + normalized_sub_issues.append(normalized_sub_issue) + if normalized_sub_issues: + normalized_ticket["sub_issues"] = normalized_sub_issues + + return normalized_ticket + + +def _ticket_has_prompt_content(ticket): + return any(ticket.get(field) for field in TICKET_PROMPT_FIELDS) + + +def _extract_related_tickets_for_prompt(tickets): + related_tickets = [] + for ticket in tickets or []: + normalized_ticket = _normalize_ticket_for_prompt(ticket) + for sub_issue in normalized_ticket.get("sub_issues", []): + prompt_sub_issue = {field: sub_issue.get(field, "") for field in TICKET_PROMPT_FIELDS} + if _ticket_has_prompt_content(prompt_sub_issue): + related_tickets.append(prompt_sub_issue) + + prompt_ticket = {field: normalized_ticket.get(field, "") for field in TICKET_PROMPT_FIELDS} + if _ticket_has_prompt_content(prompt_ticket): + related_tickets.append(prompt_ticket) + + return related_tickets + + def find_jira_tickets(text): # Regular expression patterns for JIRA tickets patterns = [ @@ -124,7 +176,8 @@ async def extract_tickets(git_provider) -> tuple[list | None, str]: sub_issues_content.append({ 'ticket_url': sub_issue_url, 'title': sub_issue.title, - 'body': sub_body + 'body': sub_body, + 'labels': "", }) except Exception as e: get_logger().warning(f"Failed to fetch sub-issue content for {sub_issue_url}: {e}") @@ -191,6 +244,7 @@ async def extract_and_cache_pr_tickets(git_provider, vars): if not get_settings().get('pr_reviewer.require_ticket_analysis_review', False): return + vars['related_tickets'] = [] vars['ticket_compliance_note'] = "" related_tickets = get_settings().get('related_tickets', []) @@ -203,13 +257,7 @@ async def extract_and_cache_pr_tickets(git_provider, vars): get_settings().set('ticket_compliance_note', vars['ticket_compliance_note']) if tickets_content: - # Store sub-issues along with main issues - for ticket in tickets_content: - if "sub_issues" in ticket and ticket["sub_issues"]: - for sub_issue in ticket["sub_issues"]: - related_tickets.append(sub_issue) # Add sub-issues content - - related_tickets.append(ticket) + related_tickets = _extract_related_tickets_for_prompt(tickets_content) get_logger().info("Extracted tickets and sub-issues from PR description", artifact={"tickets": related_tickets}) @@ -217,8 +265,10 @@ async def extract_and_cache_pr_tickets(git_provider, vars): vars['related_tickets'] = related_tickets get_settings().set('related_tickets', related_tickets) else: + related_tickets = _extract_related_tickets_for_prompt(related_tickets) get_logger().info("Using cached tickets", artifact={"tickets": related_tickets}) vars['related_tickets'] = related_tickets + get_settings().set('related_tickets', related_tickets) vars['ticket_compliance_note'] = cached_ticket_compliance_note or '' diff --git a/tests/unittest/test_convert_to_markdown.py b/tests/unittest/test_convert_to_markdown.py index c19c5236da..73f9882a78 100644 --- a/tests/unittest/test_convert_to_markdown.py +++ b/tests/unittest/test_convert_to_markdown.py @@ -3,7 +3,9 @@ import textwrap from unittest.mock import Mock -from pr_agent.algo.utils import PRReviewHeader, convert_to_markdown_v2 +from pr_agent.algo.utils import (PRReviewHeader, convert_to_markdown_v2, + normalize_ticket_requirement_text, + parse_requirement_items) from pr_agent.tools.pr_description import insert_br_after_x_chars """ @@ -247,6 +249,50 @@ def test_ticket_compliance_ignores_none_placeholders(self): assert 'Compliant requirements:' not in result assert '(none)' not in result + def test_ticket_compliance_ignores_none_with_punctuation(self): + input_data = {'review': { + 'ticket_compliance_check': [ + { + 'ticket_url': 'https://example.com/ticket/LOG-11264', + 'ticket_requirements': '- Requirement 1\n', + 'fully_compliant_requirements': '- Requirement 1\n', + 'not_compliant_requirements': 'None.\n', + 'requires_further_human_verification': '- No information provided.\n', + } + ] + }} + + result = convert_to_markdown_v2(input_data) + + assert '**🎫 Ticket compliance analysis ✅**' in result + assert '**[LOG-11264](https://example.com/ticket/LOG-11264) - Fully compliant**' in result + assert 'Compliant requirements:' in result + assert 'Non-compliant requirements:' not in result + assert 'Requires further human verification:' not in result + assert 'None.' not in result + assert 'No information provided.' not in result + + def test_parse_requirement_items_filters_placeholder_variants(self): + items = parse_requirement_items( + '- Requirement 1\n' + '• None.\n' + '1. N/A:\n' + '[ ] No information provided.\n' + '- Requirement 2\n' + ) + + assert items == ['- Requirement 1', '- Requirement 2'] + + def test_normalize_ticket_requirement_text_joins_filtered_items(self): + normalized = normalize_ticket_requirement_text( + '- Requirement 1\n' + '-\n' + '* None.\n' + '2) Requirement 2\n' + ) + + assert normalized == '- Requirement 1\n2) Requirement 2' + def test_convert_to_markdown_does_not_mutate_review_input(self): input_data = {'review': { 'score': '78/100', diff --git a/tests/unittest/test_jira_cloud_ticket_context.py b/tests/unittest/test_jira_cloud_ticket_context.py index 23f69d2d17..6ca52301c9 100644 --- a/tests/unittest/test_jira_cloud_ticket_context.py +++ b/tests/unittest/test_jira_cloud_ticket_context.py @@ -106,6 +106,7 @@ async def fake_fetch_issue_json(session, auth, key): assert ticket["sub_issues"][0]["ticket_url"] == "https://example.atlassian.net/browse/SUB-2" assert ticket["sub_issues"][0]["title"] == "Subtask 1" assert ticket["sub_issues"][0]["body"] == "" + assert ticket["sub_issues"][0]["labels"] == "" def test_fetch_jira_ticket_context_uses_scoped_token_with_cloud_id(self, monkeypatch): monkeypatch.setenv("JIRA_BASE_URL", "https://example.atlassian.net") diff --git a/tests/unittest/test_pr_reviewer_ticket_note.py b/tests/unittest/test_pr_reviewer_ticket_note.py index a9ca14d2f4..dbdbe7213c 100644 --- a/tests/unittest/test_pr_reviewer_ticket_note.py +++ b/tests/unittest/test_pr_reviewer_ticket_note.py @@ -80,6 +80,19 @@ def test_build_suspected_ticket_mismatch_note_ignores_none_placeholders(): assert "wrong Jira ticket" in result +def test_build_suspected_ticket_mismatch_note_ignores_none_with_punctuation(): + result = build_suspected_ticket_mismatch_note( + [ + { + "fully_compliant_requirements": "", + "not_compliant_requirements": "None.\n", + } + ] + ) + + assert result == "" + + def test_build_suspected_ticket_mismatch_note_skips_non_dict_entries(): result = build_suspected_ticket_mismatch_note( [ diff --git a/tests/unittest/test_ticket_pr_compliance_check.py b/tests/unittest/test_ticket_pr_compliance_check.py index fe364be8a4..c2bbc6090b 100644 --- a/tests/unittest/test_ticket_pr_compliance_check.py +++ b/tests/unittest/test_ticket_pr_compliance_check.py @@ -1,5 +1,7 @@ import asyncio +from jinja2 import Environment, StrictUndefined + from pr_agent.config_loader import get_settings from pr_agent.tools import ticket_pr_compliance_check @@ -13,6 +15,23 @@ def get_pr_branch(self): return self._branch +def _render_pr_description_prompt(related_tickets): + return Environment(undefined=StrictUndefined).from_string(get_settings().pr_description_prompt.user).render( + { + "related_tickets": related_tickets, + "title": "", + "description": "", + "branch": "", + "commit_messages_str": "", + "diff": "", + "enable_pr_diagram": False, + "enable_semantic_files_types": False, + "include_file_summary_changes": False, + "duplicate_prompt_examples": False, + } + ) + + def test_extract_tickets_prefers_title_ticket_on_mismatch(monkeypatch): async def fake_fetch_jira_ticket_context(pr_title, branch): if pr_title == "FAKE-1234 add sample feature flow": @@ -69,7 +88,7 @@ async def fake_extract_tickets(_git_provider): try: settings.set("pr_reviewer.require_ticket_analysis_review", True) - settings.set("related_tickets", []) + settings.set("related_tickets", None) settings.set("ticket_compliance_note", "") first_vars = {} @@ -156,3 +175,86 @@ async def fake_fetch_jira_ticket_context(pr_title, branch): assert "MOCK-5678" in note assert "not found" in note assert note.count("PR title references Jira ticket") == 1 + + +def test_extract_and_cache_pr_tickets_normalizes_sub_issues_for_prompt_rendering(monkeypatch): + async def fake_extract_tickets(_git_provider): + return ( + [ + { + "ticket_id": "FAKE-5678", + "ticket_url": "https://example.com/FAKE-5678", + "title": "Parent ticket", + "body": "", + "sub_issues": [{"ticket_url": "https://example.com/FAKE-5679", "title": "Child ticket", "body": ""}], + } + ], + "cached note", + ) + + monkeypatch.setattr(ticket_pr_compliance_check, "extract_tickets", fake_extract_tickets) + + settings = get_settings() + previous_require_ticket_analysis = settings.get("pr_reviewer.require_ticket_analysis_review", False) + previous_related_tickets = settings.get("related_tickets", []) + previous_ticket_note = settings.get("ticket_compliance_note", "") + + try: + settings.set("pr_reviewer.require_ticket_analysis_review", True) + settings.set("related_tickets", None) + settings.set("ticket_compliance_note", "") + + vars = {} + asyncio.run(ticket_pr_compliance_check.extract_and_cache_pr_tickets(object(), vars)) + + assert vars["related_tickets"] == [ + { + "ticket_id": "", + "ticket_url": "https://example.com/FAKE-5679", + "title": "Child ticket", + "body": "", + "labels": "", + "requirements": "", + }, + { + "ticket_id": "FAKE-5678", + "ticket_url": "https://example.com/FAKE-5678", + "title": "Parent ticket", + "body": "", + "labels": "", + "requirements": "", + }, + ] + assert _render_pr_description_prompt(vars["related_tickets"]) + finally: + settings.set("pr_reviewer.require_ticket_analysis_review", previous_require_ticket_analysis) + settings.set("related_tickets", previous_related_tickets) + settings.set("ticket_compliance_note", previous_ticket_note) + + +def test_extract_and_cache_pr_tickets_skips_empty_tickets(monkeypatch): + async def fake_extract_tickets(_git_provider): + return ([{}, {"sub_issues": [{}]}], "cached note") + + monkeypatch.setattr(ticket_pr_compliance_check, "extract_tickets", fake_extract_tickets) + + settings = get_settings() + previous_require_ticket_analysis = settings.get("pr_reviewer.require_ticket_analysis_review", False) + previous_related_tickets = settings.get("related_tickets", []) + previous_ticket_note = settings.get("ticket_compliance_note", "") + + try: + settings.set("pr_reviewer.require_ticket_analysis_review", True) + settings.set("related_tickets", None) + settings.set("ticket_compliance_note", "") + + vars = {} + asyncio.run(ticket_pr_compliance_check.extract_and_cache_pr_tickets(object(), vars)) + + assert vars["related_tickets"] == [] + assert vars["ticket_compliance_note"] == "cached note" + assert _render_pr_description_prompt(vars["related_tickets"]) + finally: + settings.set("pr_reviewer.require_ticket_analysis_review", previous_require_ticket_analysis) + settings.set("related_tickets", previous_related_tickets) + settings.set("ticket_compliance_note", previous_ticket_note)