Migrate AquaSec fetch directly into pipeline#61
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 33 minutes and 38 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRefactors the security pipeline to authenticate with AquaSec, fetch and parse findings, sync alerts to GitHub Issues, and send Teams notifications via new services and a unified orchestrator. ChangesAquaSec-Driven Security Pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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. 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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/security/conftest.py (1)
17-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove module-level docstring from this test module.
This file adds/maintains documentation at module scope in
tests/**, which violates the test-module guideline.As per coding guidelines: "
**/*.py: Do not include documentation for__init__methods and test modules".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/security/conftest.py` around lines 17 - 22, Remove the module-level docstring from this test module (the triple-quoted string at the top of tests/security/conftest.py); replace it with a short inline comment or move any necessary explanation into test-specific docstrings or external docs so the module no longer contains a top-level documentation string, ensuring no change to fixture behavior or test data mutability.src/security/services/label_checker.py (1)
59-59:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd blank line at end of file.
As per coding guidelines: Include a single blank line at the end of all Python files
🛡️ Proposed fix
labels = json.loads(result.stdout) return [entry["name"] for entry in labels if entry.get("name")] +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/security/services/label_checker.py` at line 59, Add a single blank newline character at the end of the file src/security/services/label_checker.py so the file ends with one trailing newline (ensure the file ends with exactly one blank line character after the last line of code to comply with the coding guidelines).src/security/issues/sync.py (1)
365-375:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve or explicitly migrate the
gh_alert_numberssecmeta contract.These two paths stop writing
gh_alert_numbersfor new child issues and strip it from existing ones on the next merge. That changes the serialized issue-body contract immediately, butdocs/security/security.md:54-94still documentsgh_alert_numbersin child secmeta. Any downstream parser or manual workflow following the documented format will lose that field after the first sync. Please either keep the legacy key until all consumers/docs are updated, or treat this as a coordinated breaking change in the same PR.Also applies to: 529-538
🧹 Nitpick comments (5)
tests/security/services/test_authenticator.py (1)
82-88: ⚡ Quick winMove
requeststo the module imports.The local import breaks the test-file import rule and makes the module harder to scan for dependencies. As per coding guidelines,
tests/**/*.py: Place all imports at the top of test files, never inside test functions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/security/services/test_authenticator.py` around lines 82 - 88, The test test_authenticate_raises_system_exit_on_request_exception currently imports requests inside the function; move the local "import requests" to the module-level imports at the top of tests/security/services/test_authenticator.py so all imports are declared once per file. Update the file imports block and remove the in-function import, keeping the existing mocker.patch call that references "security.services.authenticator.requests.post" and the same side_effect setup.tests/security/services/test_scan_fetcher.py (1)
114-121: ⚡ Quick winMove
requeststo the module imports.This local import breaks the test-file import rule and makes the module harder to scan for dependencies. As per coding guidelines,
tests/**/*.py: Place all imports at the top of test files, never inside test functions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/security/services/test_scan_fetcher.py` around lines 114 - 121, The test function test_fetch_findings_raises_system_exit_on_request_exception contains a local import of requests; move that import to the top-level module imports in tests/security/services/test_scan_fetcher.py so all dependencies are declared at file scope, leaving the test body to only construct ScanFetcher and apply mocker.patch on "security.services.scan_fetcher.requests.get" (referencing the ScanFetcher class and the test function name to locate the spot).src/security/services/scan_fetcher.py (1)
74-76: ⚡ Quick winPrefix the debug log consistently.
Line 76 is the only log in this flow without the security-domain prefix, which makes log filtering inconsistent. As per coding guidelines,
src/**/*.py: All logs must start with " -" prefix (e.g., "Security -").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/security/services/scan_fetcher.py` around lines 74 - 76, The debug log in scan_fetcher.py at the block using page_num, page_response and total_expected calls logger.debug without the required "Security -" prefix; update that logger.debug call to start the message with "Security - " (e.g., "Security - Expected %d total findings.") so it follows the project's logging prefix convention and remains consistent with other Security logs.tests/security/services/test_issue_syncer.py (2)
17-17: ⚡ Quick winRemove the test-module docstring.
This repo explicitly avoids documentation blocks in test modules, so this new file should start with imports instead.
As per coding guidelines, "Do not include documentation for
__init__methods and test modules".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/security/services/test_issue_syncer.py` at line 17, Remove the module-level docstring at the top of tests/security/services/test_issue_syncer.py so the file begins with import statements; ensure no other top-level documentation remains in the test module (tests should start with imports and then test code, per the guideline that test modules must not contain docstrings).
80-87: ⚡ Quick winReplace this tautological test with a behavior check.
assert result is not Nonewill pass for almost any implementation, so it doesn't verify the "does not send notifications" claim. Either assert thatIssueSyncer.sync()returns the mockedSyncResultunchanged, or drop this case as redundant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/security/services/test_issue_syncer.py` around lines 80 - 87, The test test_sync_does_not_send_notifications is tautological because it only asserts result is not None; update it to assert specific behavior: when gh_issue_list_by_label is mocked to {} and sync_alerts_and_issues returns a MagicMock SyncResult with notifications=["n"], severity_changes=[], verify that IssueSyncer.sync(...) returns that same SyncResult (or alternatively assert the returned object's notifications list is empty if the intent is "does not send notifications"). Locate the test function and replace the generic assert with an equality/assertion against the mocked SyncResult (or an assertion on result.notifications) to make the test verify the intended behavior of IssueSyncer.sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/security/aquasec-night-scan-example.yml`:
- Line 43: Replace the mutable tag in the example reusable-workflow reference
with an immutable commit SHA: change the uses line that currently reads
"AbsaOSS/organizational-workflows/.github/workflows/aquasec-scan.yml@v1.0.0" to
point to a specific commit SHA for the target repo (i.e.,
".github/workflows/aquasec-scan.yml@<commit-SHA>") so the example demonstrates
pinning to an immutable ref; update the line in
docs/security/aquasec-night-scan-example.yml where the uses: string appears (the
comment on that line can note to replace with the real SHA).
In `@docs/security/security.md`:
- Around line 34-36: Remove the stale gh_alert_numbers field from the example
child-issue payload in the security docs: locate the example payload that
contains gh_alert_numbers=["7"] (the "child-issue" example) and delete that
field and any trailing commas so the JSON/YAML remains valid; also scan the same
document for any other references to gh_alert_numbers and remove or replace them
with the current AquaSec-specific fields so the example matches the new AquaSec
Code Security API contract.
In `@src/security/alerts/aquasec_parser.py`:
- Around line 44-59: references_list is not normalized before using it for
help_uri and may be a non-list (string/other), causing incorrect values; update
parsing so you normalize rule_details.references into a canonical list once
(e.g., ensure references_list is a list) and then use that normalized list for
both references_list and when setting AlertMetadata.help_uri (use first element
if present), adjusting the code around references_list and AlertMetadata(...) /
help_uri to consume the canonical list safely.
In `@src/security/config.py`:
- Around line 56-60: The except clause handling conversion of project_number_raw
is using Python 2 comma syntax; update the exception handler in the block where
project_number is set (referencing project_number_raw, project_number) to catch
multiple exceptions with a tuple — i.e. replace the old "except ValueError,
TypeError:" with "except (ValueError, TypeError):" so both ValueError and
TypeError are properly caught under Python 3.
In `@src/security/main.py`:
- Around line 119-120: The log message from LabelChecker.check_labels() must be
prefixed with the module logging prefix; update the logger.error call that
currently logs "Required labels missing in %s: %s" to include LOGGING_PREFIX
(e.g., logger.error("%sRequired labels missing in %s: %s", LOGGING_PREFIX, repo,
", ".join(missing))). Ensure LOGGING_PREFIX is referenced (and available) in
this module and use the same logger.error formatting style as other messages in
this file.
In `@src/security/services/__init__.py`:
- Line 17: The module docstring in src/security/services/__init__.py uses an en
dash character in the text "Security services package – AquaSec API
integration."; replace that en dash with a plain ASCII hyphen (-) so the
docstring reads "Security services package - AquaSec API integration." to
satisfy Ruff RUF002 and avoid ambiguous Unicode; update the module-level string
in __init__.py accordingly.
In `@src/security/services/authenticator.py`:
- Around line 69-70: The token request currently mints an all-endpoints token by
setting allowed_endpoints=["ANY:*"]; change post_body so allowed_endpoints only
contains the minimal read endpoint(s the job actually calls (the scan-results
fetch used by src/security/services/scan_fetcher.py) instead of a wildcard.
Locate the post_body construction in authenticator.py (symbol: post_body) and
replace ["ANY:*"] with the exact HTTP method+path(s) used by the ScanFetcher
(e.g., the GET scan-results endpoint(s) invoked by the fetch function), keeping
group_id and validity logic unchanged.
- Around line 92-94: The code calls response.json() directly to get
bearer_token; wrap the JSON parsing in a try/except around the response.json()
call (the line that sets bearer_token) and catch JSON parsing errors
(requests.exceptions.JSONDecodeError and fallback ValueError) and in the except
raise SystemExit with the same auth-failure message ("ERROR: AquaSec API
response missing bearer token data.") so JSON decode failures follow the same
auth error path; keep the existing check for empty bearer_token after the try
block.
In `@src/security/services/label_checker.py`:
- Line 55: Update the log call in label_checker.py to prepend the required
domain prefix constant LOGGING_PREFIX to the error message: replace the current
logger.error invocation that logs "gh label list failed for %s:\n%s" with a
message that begins with LOGGING_PREFIX (e.g., f"{LOGGING_PREFIX} - ..."),
keeping the same placeholders for self.repo and result.stderr; modify the
logger.error call inside the method where the current line uses self.repo and
result.stderr so the domain prefix is included in the logged string.
In `@tests/security/alerts/test_aquasec_parser.py`:
- Around line 202-207: The assertions in test_parse_item_empty_extra_data
mismatch the current parser behavior: _parse_item returns empty strings for
missing fields, so update the test expectations on alert.rule_details.owasp and
alert.rule_details.references to assert "" (empty string) instead of "N/A" to
match the behavior of _parse_item and keep rule_details.cwe assertion as-is.
In `@tests/security/services/__init__.py`:
- Line 17: Remove the module-level docstring (the triple-quoted string at the
top of the tests.security.services.__init__ module) so the test package's
__init__ contains no documentation; simply delete that string literal and keep
any necessary imports or __all__ intact.
In `@tests/security/services/test_label_checker.py`:
- Line 43: Tests use the incorrect assert order (actual == expected); change
them to the canonical assert expected == actual pattern by flipping each
comparison, e.g. replace assertions like assert checker._fetch_labels() ==
["scope:security", "epic"] with assert ["scope:security", "epic"] ==
checker._fetch_labels(), and apply the same flip for the other occurrences
referenced (lines using checker._fetch_labels() or similar actual-first
comparisons at the other mentioned locations).
In `@tests/security/services/test_notification_sender.py`:
- Around line 19-23: A local import of the requests module exists inside a test
function; move the import for requests to module scope with the other imports at
the top of the file (next to pytest, MagicMock, NotifiedIssue, SeverityChange,
and NotificationSender) so tests follow the rule of placing all imports at the
top; update any test that currently does "import requests" inside the function
(around the block that references
NotificationSender/NotifiedIssue/SeverityChange) to use the module-level
requests import.
In `@tests/security/test_main.py`:
- Around line 30-36: The autouse fixture _aqua_env sets Aqua env vars but
doesn't stub the external dependency "gh", so main() can exit early on runners
without gh; update the _aqua_env fixture to also stub availability of the GitHub
CLI by monkeypatching the lookup used by your code (e.g.,
monkeypatch.setattr(shutil, "which", lambda name: "/usr/bin/gh" if name == "gh"
else None) or monkeypatch.setenv behavior your code relies on) so that when
tests call main() it sees "gh" as present; modify the fixture _aqua_env to
include that monkeypatch call so all tests in this module see a fake gh.
---
Outside diff comments:
In `@src/security/services/label_checker.py`:
- Line 59: Add a single blank newline character at the end of the file
src/security/services/label_checker.py so the file ends with one trailing
newline (ensure the file ends with exactly one blank line character after the
last line of code to comply with the coding guidelines).
In `@tests/security/conftest.py`:
- Around line 17-22: Remove the module-level docstring from this test module
(the triple-quoted string at the top of tests/security/conftest.py); replace it
with a short inline comment or move any necessary explanation into test-specific
docstrings or external docs so the module no longer contains a top-level
documentation string, ensuring no change to fixture behavior or test data
mutability.
---
Nitpick comments:
In `@src/security/services/scan_fetcher.py`:
- Around line 74-76: The debug log in scan_fetcher.py at the block using
page_num, page_response and total_expected calls logger.debug without the
required "Security -" prefix; update that logger.debug call to start the message
with "Security - " (e.g., "Security - Expected %d total findings.") so it
follows the project's logging prefix convention and remains consistent with
other Security logs.
In `@tests/security/services/test_authenticator.py`:
- Around line 82-88: The test
test_authenticate_raises_system_exit_on_request_exception currently imports
requests inside the function; move the local "import requests" to the
module-level imports at the top of tests/security/services/test_authenticator.py
so all imports are declared once per file. Update the file imports block and
remove the in-function import, keeping the existing mocker.patch call that
references "security.services.authenticator.requests.post" and the same
side_effect setup.
In `@tests/security/services/test_issue_syncer.py`:
- Line 17: Remove the module-level docstring at the top of
tests/security/services/test_issue_syncer.py so the file begins with import
statements; ensure no other top-level documentation remains in the test module
(tests should start with imports and then test code, per the guideline that test
modules must not contain docstrings).
- Around line 80-87: The test test_sync_does_not_send_notifications is
tautological because it only asserts result is not None; update it to assert
specific behavior: when gh_issue_list_by_label is mocked to {} and
sync_alerts_and_issues returns a MagicMock SyncResult with notifications=["n"],
severity_changes=[], verify that IssueSyncer.sync(...) returns that same
SyncResult (or alternatively assert the returned object's notifications list is
empty if the intent is "does not send notifications"). Locate the test function
and replace the generic assert with an equality/assertion against the mocked
SyncResult (or an assertion on result.notifications) to make the test verify the
intended behavior of IssueSyncer.sync.
In `@tests/security/services/test_scan_fetcher.py`:
- Around line 114-121: The test function
test_fetch_findings_raises_system_exit_on_request_exception contains a local
import of requests; move that import to the top-level module imports in
tests/security/services/test_scan_fetcher.py so all dependencies are declared at
file scope, leaving the test body to only construct ScanFetcher and apply
mocker.patch on "security.services.scan_fetcher.requests.get" (referencing the
ScanFetcher class and the test function name to locate the spot).
🪄 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: 3c13bd5c-6f28-4818-b746-54733ff7ed4f
📒 Files selected for processing (42)
.github/copilot-instructions.md.github/workflows/aquasec-night-scan.yml.github/workflows/aquasec-scan.ymldocs/security/aquasec-night-scan-example.ymldocs/security/security.mdsrc/security/README.mdsrc/security/alerts/aquasec_parser.pysrc/security/alerts/models.pysrc/security/alerts/parser.pysrc/security/check_labels.pysrc/security/collect_alert.pysrc/security/config.pysrc/security/constants.pysrc/security/issues/secmeta.pysrc/security/issues/sync.pysrc/security/main.pysrc/security/notifications/teams.pysrc/security/promote_alerts.pysrc/security/send_notifications.pysrc/security/services/__init__.pysrc/security/services/authenticator.pysrc/security/services/issue_syncer.pysrc/security/services/label_checker.pysrc/security/services/notification_sender.pysrc/security/services/scan_fetcher.pytests/security/alerts/test_aquasec_parser.pytests/security/alerts/test_parser.pytests/security/conftest.pytests/security/issues/test_secmeta.pytests/security/issues/test_sync.pytests/security/notifications/test_teams.pytests/security/services/__init__.pytests/security/services/test_authenticator.pytests/security/services/test_issue_syncer.pytests/security/services/test_label_checker.pytests/security/services/test_notification_sender.pytests/security/services/test_scan_fetcher.pytests/security/test_collect_alert.pytests/security/test_config.pytests/security/test_main.pytests/security/test_promote_alerts.pytests/security/test_send_notifications.py
💤 Files with no reviewable changes (14)
- tests/security/test_promote_alerts.py
- src/security/check_labels.py
- src/security/alerts/parser.py
- .github/workflows/aquasec-night-scan.yml
- src/security/send_notifications.py
- src/security/issues/secmeta.py
- tests/security/notifications/test_teams.py
- src/security/notifications/teams.py
- tests/security/alerts/test_parser.py
- tests/security/test_send_notifications.py
- src/security/collect_alert.py
- src/security/promote_alerts.py
- tests/security/test_collect_alert.py
- tests/security/issues/test_secmeta.py
# Conflicts: # .github/workflows/aquasec-scan.yml
Collaborator
Author
|
Ready for human review @miroslavpojer. OLD section means a generated issues, that were created with the master (old) logic. NEW means issues generated with the logic in this PR. OLD NEW In the new issues are little changes (all already existing issues will have to be updated), but overall output should be almost the same!! |
miroslavpojer
approved these changes
Jun 1, 2026
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 security automation pipeline to fetch and process AquaSec scan results directly from the AquaSec API, rather than relying on GitHub Code Scanning alerts. It updates documentation, workflow configuration, and the codebase to reflect this new architecture, and introduces a dedicated parser for AquaSec findings. The changes clarify the authentication and data flow, streamline environment variable usage, and update user instructions accordingly.
Release Notes
Related
Closes #37
Closes #57
Summary by CodeRabbit
New Features
Documentation
Tests