Security issues rendering body update#47
Open
tmikula-dev wants to merge 15 commits intomasterfrom
Open
Conversation
# Conflicts: # .github/workflows/remove-adept-to-close-on-issue-close.yml
tmikula-dev
added
the
work in progress
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughReplaces AVD/CVE fields with rule-based identifiers and descriptions, adds Markdown normalization and N/A section stripping, updates alert parsing to support multi-line fields, restructures issue templates to include rule_id, advisory_url, and explicit line numbers, and adjusts related tests and workflow checkout ref. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 43 minutes and 10 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
.github/copilot-instructions.md (1)
44-44: Clarify the docstring restriction at Line 44 (minor readability).Line 44 correctly reflects both rules (“no
__init__doc” and “no test module docs”), but it’s slightly ambiguous as a single sentence. Consider splitting into two bullets to make the scope obvious (src vs tests).Suggested tweak
- No documentation for `__init__` methods and test modules + - No documentation for `__init__` methods (applies to `src/**/*.py`) + - No documentation for test modules (applies to `tests/**/*.py`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/copilot-instructions.md at line 44, The docstring rule at the line mentioning "No documentation for `__init__` methods and test modules" is ambiguous; split it into two separate bullets so one clearly states "Do not add docstrings to __init__ methods in source modules" and the other states "Do not add docstrings to test modules", clarifying scope (source vs tests) and keeping the original intent; update the text near the `__init__` reference in .github/copilot-instructions.md accordingly to use two distinct bullets and mirror existing wording style.tests/security/issues/test_secmeta.py (1)
119-120: Rename ambiguous variablelto improve readability.The variable name
l(lowercase L) is flagged by Ruff (E741) as potentially confusing with1(one) orI(capital i). Uselineinstead.♻️ Proposed refactor
- type_idx = next(i for i, l in enumerate(lines) if "type=" in l) - fp_idx = next(i for i, l in enumerate(lines) if "fingerprint=" in l) + type_idx = next(i for i, line in enumerate(lines) if "type=" in line) + fp_idx = next(i for i, line in enumerate(lines) if "fingerprint=" in line)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/issues/test_secmeta.py` around lines 119 - 120, Rename the ambiguous loop variable `l` to `line` in the generator expressions that compute `type_idx` and `fp_idx` to satisfy Ruff E741 and improve readability: update the expressions using `next(i for i, l in enumerate(lines) if "type=" in l)` and `next(i for i, l in enumerate(lines) if "fingerprint=" in l)` to use `line` instead (`next(i for i, line in enumerate(lines) if "type=" in line)` and similarly for fingerprint) so the variables `type_idx`, `fp_idx`, and the `lines` enumeration remain correct and clear.tests/security/test_collect_alert.py (1)
150-190: Parameterize the multiline parser coverage and includeOWASP.
src/security/collect_alert.pytreats bothreferencesandowaspas multiline fields, but this new coverage only exercisesReferences. A regression inOWASPparsing would currently slip through while breaking the new parent-body section.As per coding guidelines "Use
@pytest.mark.parametrizefor data-driven tests, especially for negative/failure scenarios with multiple similar cases".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/test_collect_alert.py` around lines 150 - 190, Tests only exercise multiline parsing for "References" so regressions in parsing other multiline fields like "OWASP" can slip by; update the tests to use pytest.mark.parametrize to cover both "References" and "OWASP" (and include negative/empty cases) by calling _help_multiline_value with each field name and verifying expected results, and extend test_parse_rule_details_multiline_references to include a case where rule_help contains an "**OWASP:**" multiline list and assert that _parse_rule_details returns entries under details["owasp"] as well; reference the helper _help_multiline_value and parser _parse_rule_details when adding the new parametrized cases.tests/security/issues/test_builder.py (2)
219-223: Assert the full title here, not a substring.
build_issue_title()is deterministic, soexpected in ...will still pass if the function starts appending extra text or whitespace. This should be an exact equality check. As per coding guidelines,tests/**/*.py: Use assertion patternassert expected == actualin tests.Proposed fix
def test_build_issue_title( description: str | None, rule_name: str | None, rule_id: str, fingerprint: str, expected: str ) -> None: - assert expected in build_issue_title(description, rule_name, rule_id, fingerprint) + assert expected == build_issue_title(description, rule_name, rule_id, fingerprint)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/issues/test_builder.py` around lines 219 - 223, The test test_build_issue_title should assert exact equality instead of substring membership: replace the current assertion in test_build_issue_title that uses "expected in build_issue_title(...)" with an equality check such as "assert expected == build_issue_title(description, rule_name, rule_id, fingerprint)" (or assign actual = build_issue_title(...) then assert expected == actual) so the test fails if extra text/whitespace is appended; reference build_issue_title and test_build_issue_title when making the change.
306-309: Make this assertion target the date fallback explicitly.
count("N/A") >= 1can still pass because other missing fields renderN/A, even if thefirst_seenfallback regresses. Assert on the specific rendered field or section instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/issues/test_builder.py` around lines 306 - 309, The test test_first_seen_yields_na_when_no_fallback currently uses a loose assertion on build_child_issue_body(alert).count("N/A") which can pass for unrelated missing fields; change it to assert the specific rendered first-seen field instead by checking build_child_issue_body(alert) contains the exact first-seen output (e.g., "First seen: N/A" or the exact label/section your renderer emits for the first_seen fallback) so the assertion targets the first_seen fallback behavior of build_child_issue_body and will fail if that specific rendering regresses.src/core/github/projects.py (3)
52-65: Apply logging guidelines: use lazy % formatting and domain prefix.The coding guidelines specify:
- Use lazy % formatting in logging:
logger.warning("msg %s", var)instead of f-strings- All logs must start with " -" prefix (e.g., "Projects -")
♻️ Proposed fix
def _run_graphql(query: str, variables: dict[str, Any] | None = None) -> dict[str, Any] | None: """Execute a GraphQL query via ``gh api graphql`` and return parsed JSON.""" args = ["api", "graphql", "-f", f"query={query}"] for k, v in (variables or {}).items(): args += ["-F", f"{k}={v}"] res = run_gh(args) if res.returncode != 0: - logging.warning(f"GraphQL call failed: {res.stderr}") + logger.warning("Projects - GraphQL call failed: %s", res.stderr) return None try: return json.loads(res.stdout) except Exception: - logging.warning(f"Could not parse GraphQL response: {res.stdout!r}") + logger.warning("Projects - Could not parse GraphQL response: %r", res.stdout) return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/github/projects.py` around lines 52 - 65, In _run_graphql, change the two logging.warn calls to use lazy % formatting and add the "Projects -" domain prefix: replace logging.warning(f"GraphQL call failed: {res.stderr}") with logging.warning("Projects - GraphQL call failed: %s", res.stderr) (when res.returncode != 0), and replace logging.warning(f"Could not parse GraphQL response: {res.stdout!r}") inside the except block with logging.warning("Projects - Could not parse GraphQL response: %r", res.stdout); keep the rest of the function (args construction and run_gh usage) unchanged.
17-29: Uselogging.getLogger(__name__)for logging.The coding guidelines for
src/**/*.pyfiles require usinglogging.getLogger(__name__)for logging rather than using the module-levelloggingdirectly.♻️ Proposed fix
import json import logging from dataclasses import dataclass from typing import Any +logger = logging.getLogger(__name__) + from .client import run_gh from ..priority import resolve_priority🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/github/projects.py` around lines 17 - 29, Replace module-level direct calls to the logging module with a module-scoped logger created via logging.getLogger(__name__); add a line creating logger = logging.getLogger(__name__) near the imports and update all uses in this module (e.g., any calls around functions/classes that call run_gh or use resolve_priority or ProjectPrioritySync) to use logger.info/warning/error/debug instead of logging.info/warning/error/debug so the module follows the project logging guideline.
68-134: Apply logging guidelines consistently throughout the file.The file has pervasive use of f-strings in logging calls and uses
loggingmodule directly instead of a module-level logger. The coding guidelines specify lazy % formatting and"<Domain> -"prefix for logs.♻️ Proposed fixes for logging patterns
Add at module level after imports:
logger = logging.getLogger(__name__)Example fixes for lines 99-102:
logging.debug(f"Querying ProjectV2 #{project_number} in org '{org}'") + logger.debug("Projects - Querying ProjectV2 #%s in org '%s'", project_number, org) data = _run_graphql(query, {"org": org, "num": project_number}) if data is None: - logging.warning(f"GraphQL query for project #{project_number} in org '{org}' failed.") + logger.warning("Projects - GraphQL query for project #%s in org '%s' failed.", project_number, org)Apply similar pattern to all
logging.debug/warning/infocalls throughout the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/github/projects.py` around lines 68 - 134, Add a module-level logger (e.g., logger = logging.getLogger(__name__)) after imports and replace direct logging.* calls in gh_project_get_priority_field and other functions that reference _project_priority_cache, _run_graphql, and ProjectPriorityField to use that logger; change f-string messages to lazy %-formatting and prepend the domain prefix (e.g., "GitHub Projects - ") to each message (e.g., logger.debug("GitHub Projects - Querying ProjectV2 #%d in org '%s'", project_number, org)). Ensure all logging.debug/warning/info calls in this file follow the same pattern.src/core/github/client.py (1)
32-43: Uselogging.getLogger(__name__)instead of module-levellogging.The coding guidelines for
src/**/*.pyfiles specify usinglogging.getLogger(__name__)for logging. Currently the module importsloggingdirectly and useslogging.error(). Create a module-level logger instead.♻️ Proposed fix
import logging import subprocess + + +logger = logging.getLogger(__name__) def run_cmd(And update line 42:
try: return run_cmd(cmd, capture_output=capture_output) except FileNotFoundError as exc: - logging.error("gh CLI not found. Install and authenticate gh.") + logger.error("gh CLI not found. Install and authenticate gh.") raise SystemExit(1) from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/github/client.py` around lines 32 - 43, The except block in run_gh uses the root logging module; add a module-level logger = logging.getLogger(__name__) and replace the logging.error call inside run_gh's FileNotFoundError handler with logger.error so the module follows project logging conventions; ensure the new logger is defined near other imports/definitions so run_gh uses logger.error instead of logging.error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/aquasec-scan.yml:
- Line 112: The workflow uses an invalid GitHub Actions context property
'github.job_workflow_sha'; replace it with the correct reusable-workflow
property 'job.workflow_sha' by updating the 'ref' value (change occurrences of
github.job_workflow_sha to job.workflow_sha) so the workflow references the
commit SHA of the workflow file that defines the current job.
In `@src/security/issues/builder.py`:
- Around line 55-64: The owasp and references fields are not escaped before
being inserted into PARENT_BODY_TEMPLATE; update the builder that returns the
dict (the block setting "owasp" and "references") to sanitize these values: run
owasp through sanitize_markdown (e.g., "owasp": sanitize_markdown(owasp)) and
for "references" convert the incoming list/strings into a safe structural
representation by sanitizing each item (e.g., map sanitize_markdown over
alert.rule_details.references or references variable) and join or render as a
safe bullet-list string before assigning to "references". Ensure you reference
PARENT_BODY_TEMPLATE, the "owasp" and "references" keys, and reuse the existing
sanitize_markdown helper so markup cannot be injected.
In `@src/security/issues/templates.py`:
- Around line 47-57: The standalone sections for Remediation/OWASP/References
can render a literal "N/A" because strip_na_sections() only removes bullet-style
N/A; update either the template or the stripper to avoid that: one option is to
change the template in src/security/issues/templates.py to conditionally render
each section only when extraData.remediation, extraData.owasp, and
extraData.references are present and not equal to "N/A" (or blank);
alternatively extend the strip_na_sections() helper to treat a section body
equal to "N/A" (or whitespace-only "N/A") as empty and strip the entire
heading+body. Reference the template sections named "Remediation", "OWASP", and
"References" and the function strip_na_sections() when making the change.
In `@tests/security/issues/test_builder.py`:
- Line 325: Rename the loop variable `l` to `line` in the failing assertions in
tests/security/issues/test_builder.py so Ruff E741 is resolved; specifically
update the generator expressions used in the assertions (e.g., the one that
reads assert not any(l.strip().startswith("## Black") for l in body.split("\n"))
and the other at the second occurrence) to use `line.strip().startswith(...) for
line in body.split("\n")`, keeping the rest of the assertions and message text
unchanged.
---
Nitpick comments:
In @.github/copilot-instructions.md:
- Line 44: The docstring rule at the line mentioning "No documentation for
`__init__` methods and test modules" is ambiguous; split it into two separate
bullets so one clearly states "Do not add docstrings to __init__ methods in
source modules" and the other states "Do not add docstrings to test modules",
clarifying scope (source vs tests) and keeping the original intent; update the
text near the `__init__` reference in .github/copilot-instructions.md
accordingly to use two distinct bullets and mirror existing wording style.
In `@src/core/github/client.py`:
- Around line 32-43: The except block in run_gh uses the root logging module;
add a module-level logger = logging.getLogger(__name__) and replace the
logging.error call inside run_gh's FileNotFoundError handler with logger.error
so the module follows project logging conventions; ensure the new logger is
defined near other imports/definitions so run_gh uses logger.error instead of
logging.error.
In `@src/core/github/projects.py`:
- Around line 52-65: In _run_graphql, change the two logging.warn calls to use
lazy % formatting and add the "Projects -" domain prefix: replace
logging.warning(f"GraphQL call failed: {res.stderr}") with
logging.warning("Projects - GraphQL call failed: %s", res.stderr) (when
res.returncode != 0), and replace logging.warning(f"Could not parse GraphQL
response: {res.stdout!r}") inside the except block with
logging.warning("Projects - Could not parse GraphQL response: %r", res.stdout);
keep the rest of the function (args construction and run_gh usage) unchanged.
- Around line 17-29: Replace module-level direct calls to the logging module
with a module-scoped logger created via logging.getLogger(__name__); add a line
creating logger = logging.getLogger(__name__) near the imports and update all
uses in this module (e.g., any calls around functions/classes that call run_gh
or use resolve_priority or ProjectPrioritySync) to use
logger.info/warning/error/debug instead of logging.info/warning/error/debug so
the module follows the project logging guideline.
- Around line 68-134: Add a module-level logger (e.g., logger =
logging.getLogger(__name__)) after imports and replace direct logging.* calls in
gh_project_get_priority_field and other functions that reference
_project_priority_cache, _run_graphql, and ProjectPriorityField to use that
logger; change f-string messages to lazy %-formatting and prepend the domain
prefix (e.g., "GitHub Projects - ") to each message (e.g., logger.debug("GitHub
Projects - Querying ProjectV2 #%d in org '%s'", project_number, org)). Ensure
all logging.debug/warning/info calls in this file follow the same pattern.
In `@tests/security/issues/test_builder.py`:
- Around line 219-223: The test test_build_issue_title should assert exact
equality instead of substring membership: replace the current assertion in
test_build_issue_title that uses "expected in build_issue_title(...)" with an
equality check such as "assert expected == build_issue_title(description,
rule_name, rule_id, fingerprint)" (or assign actual = build_issue_title(...)
then assert expected == actual) so the test fails if extra text/whitespace is
appended; reference build_issue_title and test_build_issue_title when making the
change.
- Around line 306-309: The test test_first_seen_yields_na_when_no_fallback
currently uses a loose assertion on build_child_issue_body(alert).count("N/A")
which can pass for unrelated missing fields; change it to assert the specific
rendered first-seen field instead by checking build_child_issue_body(alert)
contains the exact first-seen output (e.g., "First seen: N/A" or the exact
label/section your renderer emits for the first_seen fallback) so the assertion
targets the first_seen fallback behavior of build_child_issue_body and will fail
if that specific rendering regresses.
In `@tests/security/issues/test_secmeta.py`:
- Around line 119-120: Rename the ambiguous loop variable `l` to `line` in the
generator expressions that compute `type_idx` and `fp_idx` to satisfy Ruff E741
and improve readability: update the expressions using `next(i for i, l in
enumerate(lines) if "type=" in l)` and `next(i for i, l in enumerate(lines) if
"fingerprint=" in l)` to use `line` instead (`next(i for i, line in
enumerate(lines) if "type=" in line)` and similarly for fingerprint) so the
variables `type_idx`, `fp_idx`, and the `lines` enumeration remain correct and
clear.
In `@tests/security/test_collect_alert.py`:
- Around line 150-190: Tests only exercise multiline parsing for "References" so
regressions in parsing other multiline fields like "OWASP" can slip by; update
the tests to use pytest.mark.parametrize to cover both "References" and "OWASP"
(and include negative/empty cases) by calling _help_multiline_value with each
field name and verifying expected results, and extend
test_parse_rule_details_multiline_references to include a case where rule_help
contains an "**OWASP:**" multiline list and assert that _parse_rule_details
returns entries under details["owasp"] as well; reference the helper
_help_multiline_value and parser _parse_rule_details when adding the new
parametrized cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e3bd026-6240-4a73-9c87-fc8df447bf6c
📒 Files selected for processing (30)
.github/copilot-instructions.md.github/workflows/aquasec-scan.yml.github/workflows/remove-adept-to-close-on-issue-close.ymlpyproject.tomlsrc/core/github/client.pysrc/core/github/projects.pysrc/core/helpers.pysrc/core/rendering.pysrc/security/alerts/models.pysrc/security/collect_alert.pysrc/security/constants.pysrc/security/derive_team_security_metrics.pysrc/security/extract_team_security_stats.pysrc/security/issues/builder.pysrc/security/issues/models.pysrc/security/issues/sec_events.pysrc/security/issues/secmeta.pysrc/security/issues/sync.pysrc/security/issues/templates.pytests/core/test_helpers.pytests/security/alerts/test_models.pytests/security/conftest.pytests/security/issues/test_builder.pytests/security/issues/test_models.pytests/security/issues/test_sec_events.pytests/security/issues/test_secmeta.pytests/security/issues/test_sync.pytests/security/issues/test_templates.pytests/security/test_collect_alert.pytests/security/test_constants.py
💤 Files with no reviewable changes (7)
- src/security/constants.py
- tests/security/test_constants.py
- tests/security/issues/test_sec_events.py
- .github/workflows/remove-adept-to-close-on-issue-close.yml
- src/security/issues/sec_events.py
- src/security/derive_team_security_metrics.py
- src/security/extract_team_security_stats.py
# Conflicts: # src/security/issues/builder.py # src/security/issues/sync.py # src/security/issues/templates.py # tests/security/issues/test_builder.py # tests/security/issues/test_models.py # tests/security/issues/test_templates.py
tmikula-dev
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/security/issues/builder.py (1)
137-146: Redundant conditional infile_displayderivation.
str.rsplit("/", 1)[-1]already returns the entire string when no/is present, so theif "/" in artifact else artifactternary on Line 142 is unnecessary. Thescm_filebranch on Line 144 omits this check, making the two arms inconsistent. Simplifying also removes the duplicated basename logic.♻️ Proposed simplification
- # Use artifact as the display file name, fall back to scm_file basename - artifact = alert.alert_details.artifact - scm_file = alert.alert_details.scm_file - - if artifact and artifact != NOT_AVAILABLE: - file_display = artifact.rsplit("/", 1)[-1] if "/" in artifact else artifact - elif scm_file and scm_file != NOT_AVAILABLE: - file_display = scm_file.rsplit("/", 1)[-1] - else: - file_display = NOT_AVAILABLE + # Use artifact as the display file name, fall back to scm_file basename + artifact = alert.alert_details.artifact + scm_file = alert.alert_details.scm_file + source = artifact if artifact and artifact != NOT_AVAILABLE else scm_file + if source and source != NOT_AVAILABLE: + file_display = source.rsplit("/", 1)[-1] + else: + file_display = NOT_AVAILABLE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/security/issues/builder.py` around lines 137 - 146, The file display derivation has redundant and inconsistent checks: simplify by removing the ternary that tests for "/" in artifact and instead unconditionally use artifact.rsplit("/", 1)[-1] when artifact is present and not NOT_AVAILABLE; do the same for scm_file in the elif branch (use scm_file.rsplit("/", 1)[-1]); keep the outer guards (artifact != NOT_AVAILABLE and scm_file != NOT_AVAILABLE) and the final else assigning NOT_AVAILABLE; this updates the logic around alert.alert_details.artifact, alert.alert_details.scm_file, and file_display to be concise and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/security/issues/builder.py`:
- Around line 137-146: The file display derivation has redundant and
inconsistent checks: simplify by removing the ternary that tests for "/" in
artifact and instead unconditionally use artifact.rsplit("/", 1)[-1] when
artifact is present and not NOT_AVAILABLE; do the same for scm_file in the elif
branch (use scm_file.rsplit("/", 1)[-1]); keep the outer guards (artifact !=
NOT_AVAILABLE and scm_file != NOT_AVAILABLE) and the final else assigning
NOT_AVAILABLE; this updates the logic around alert.alert_details.artifact,
alert.alert_details.scm_file, and file_display to be concise and consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c880ebae-e59d-4017-9060-9941aa4fec26
📒 Files selected for processing (8)
.github/workflows/aquasec-scan.ymlsrc/core/helpers.pysrc/security/issues/builder.pysrc/security/issues/sync.pytests/core/test_helpers.pytests/security/issues/test_builder.pytests/security/issues/test_secmeta.pytests/security/issues/test_sync.py
💤 Files with no reviewable changes (1)
- tests/security/issues/test_sync.py
✅ Files skipped from review due to trivial changes (1)
- tests/security/issues/test_secmeta.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/helpers.py
- src/security/issues/sync.py
- tests/core/test_helpers.py
tmikula-dev
removed
the
work in progress
miroslavpojer
previously approved these changes
Apr 28, 2026
Contributor
miroslavpojer
left a comment
miroslavpojer
left a comment
There was a problem hiding this comment.
Did you run it in dry run on Unify?
Collaborator
Author
|
@miroslavpojer The task is still under review with the PO, the dry-run is following. |
Contributor
I mean from cli, dry run is availavle there - as simulation that all edits are ok, no behaving unexpected. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/security/issues/builder.py`:
- Around line 31-38: The HTML link generator _new_window_link currently
interpolates raw text and url and uses target="_blank" without rel; sanitize
both values by HTML-escaping them (use html.escape) and validate/normalize the
URL to allowed schemes (e.g., http, https, mailto) to prevent javascript: or
other unsafe schemes; then return a safe anchor with rel="noopener noreferrer"
(e.g., '<a href="{escaped_url}" target="_blank" rel="noopener
noreferrer">{escaped_text}</a>'); apply the same fixes to the other identical
occurrence referenced in the review (the other site where raw link HTML is
produced).
- Line 86: The return string in the function that builds the alert title uses an
en dash (“–”) which trips Ruff RUF001; update the f-string in builder.py (the
return statement that currently reads f"Security Alert – {rule_id}".strip()) to
use an ASCII hyphen instead (f"Security Alert - {rule_id}".strip()), keeping the
existing .strip() call intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c511809-a859-4259-9064-23b7432b9548
📒 Files selected for processing (9)
.github/copilot-instructions.mdsrc/security/constants.pysrc/security/issues/builder.pysrc/security/issues/sync.pysrc/security/issues/templates.pytests/security/issues/test_builder.pytests/security/issues/test_sync.pytests/security/issues/test_templates.pytests/security/test_constants.py
💤 Files with no reviewable changes (1)
- tests/security/test_constants.py
✅ Files skipped from review due to trivial changes (3)
- src/security/constants.py
- .github/copilot-instructions.md
- tests/security/issues/test_templates.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/security/issues/test_sync.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This pull request refactors the generation of security issue Markdown bodies and their supporting code to improve clarity, maintainability, and output quality. The main changes include removing outdated fields (like
cveandavd_id), updating templates to match the new data model, introducing a utility to strip empty/N/A sections from Markdown, and enhancing parsing and formatting logic for alert details.Release Notes
Related
Closes #39
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests