Adding the dry-run#48
Open
tmikula-dev wants to merge 30 commits intomasterfrom
Open
Conversation
# Conflicts: # .github/workflows/remove-adept-to-close-on-issue-close.yml
# 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
WalkthroughAdds a dry-run input to AquaSec workflows and wires it through. Implements dry-run-aware logging and prefixes across the security alerts → issues pipeline, introduces SyncStats and extended SyncContext, refactors sync orchestration (deferred parent updates, labeling/closure), updates notifications to call an external script, and adjusts CLI/file-overwrite behavior and tests. ChangesSecurity sync + dry-run (single cohort)
Sequence DiagramsequenceDiagram
actor User
participant Main as security/main.py
participant Parser as alerts/parser.py
participant Sync as issues/sync.py
participant GitHub as GitHub API
participant Teams as notifications/teams.py
User->>Main: run (dry-run or live)
Main->>Parser: load alerts file
Parser-->>Main: parsed alerts
Main->>Sync: sync_alerts_and_issues(alerts, SyncContext)
Sync->>Sync: build IssueIndex and SyncStats
Sync->>Sync: ensure_parent_issue / ensure_child_issue (accumulate state)
Sync->>Sync: _flush_parent_body_updates(stats)
alt live run
Sync->>GitHub: apply updates (bodies, titles, links, labels)
GitHub-->>Sync: responses
else dry-run
Sync-->>Sync: log would-be operations with DRY_RUN_PREFIX
end
Sync->>Sync: _label_adept_to_close_issues()
Sync->>Sync: _log_sync_summary(stats)
Main->>Teams: notify_teams(severity_changes)
Teams-->>Teams: call send_notifications.py (with --dry-run when applicable)
Main-->>User: return result / summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
| priority_sync: ProjectPrioritySync | None = None, | ||
| severity_changes: list[SeverityChange] | None = None, | ||
| parent_original_bodies: dict[int, tuple[str, str]] | None = None, | ||
| stats: SyncStats | None = None, |
Contributor
There was a problem hiding this comment.
Why it is allowed to be None too? This is under our control, right?
Collaborator
Author
There was a problem hiding this comment.
You're right for all lines 197-199 (severity_changes, parent_original_bodies, and stats). Those are always provided. I fixed those in commit: dad4a49.
# Conflicts: # .github/copilot-instructions.md # src/security/constants.py # src/security/issues/sync.py
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/security/aquasec-night-scan-example.yml (1)
25-46:⚠️ Potential issue | 🟠 MajorUpdate the pinned commit SHA to reference a version of
aquasec-scan.ymlthat exposes thedry-runinput.The pinned commit
c1fa5b54ff24fea071415da89abc4f0506344f01references a revision ofaquasec-scan.ymlthat does not have adry-runinput defined. Users copying this example would get a workflow validation error when attempting to passdry-run: ${{ inputs.dry-run || false }}to a reusable workflow that doesn't accept that input. Update the SHA to point to a commit that includes thedry-runinput in the reusable workflow's input specification.🤖 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 `@docs/security/aquasec-night-scan-example.yml` around lines 25 - 46, The pinned commit SHA for the reusable workflow in jobs.scan (AbsaOSS/organizational-workflows/.github/workflows/aquasec-scan.yml@c1fa5b54ff24fea071415da89abc4f0506344f01) points to a revision that does not declare the dry-run input; update that SHA to a commit where the reusable workflow aquasec-scan.yml includes an inputs.dry-run definition so the with: dry-run: ${{ inputs.dry-run || false }} call is valid, ensuring the jobs.scan uses the new commit hash that exposes the dry-run input.src/security/issues/sync.py (1)
411-441:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncrement
children_linkedcounter when linking new child to parent in live mode.The dry-run branch increments
sync.stats.children_linkedwhen a parent exists (line 382), but the live branch at lines 429–441 callsgh_issue_add_sub_issue_by_number()without bumping the counter. This makeschildren_linkedunderstate reality whenever a brand-new child is auto-linked to an existing parent, diverging from the matching dry-run number in the sync summary. The counter should be incremented on success, matching the pattern used at line 629.🔧 Proposed fix
if parent_issue is not None: maybe_reopen_parent_issue( ctx.repo, parent_issue, rule_id=ctx.rule_id, dry_run=sync.dry_run, context="new_child", child_issue_number=num, ) logging.debug("Add sub-issue link parent=#%d child=#%d (alert %d)", parent_issue.number, num, ctx.alert_number) - gh_issue_add_sub_issue_by_number(ctx.repo, parent_issue.number, num) + if gh_issue_add_sub_issue_by_number(ctx.repo, parent_issue.number, num): + sync.stats.children_linked += 1🤖 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/issues/sync.py` around lines 411 - 441, The live-branch is calling gh_issue_add_sub_issue_by_number(...) to link a newly created child to a parent but does not increment sync.stats.children_linked, causing the counter to be understated; update the block in the new-child path (after maybe_reopen_parent_issue(...) and after gh_issue_add_sub_issue_by_number(...)) to increment sync.stats.children_linked on successful linking (matching the dry-run behavior and the pattern used elsewhere, e.g., line with sync.stats.children_linked increment at the dry-run branch and at line 629), ensuring the increment occurs only in non-dry-run/live execution and after the gh_issue_add_sub_issue_by_number call succeeds.
🧹 Nitpick comments (5)
src/core/config.py (1)
44-44: 💤 Low value
sys.stdout.flush()immediately afterbasicConfigis a no-op.
logging.basicConfigonly configures handlers; it does not emit any records, so there is nothing buffered to flush at this point. If the goal is real-time log visibility on GitHub Actions runners, consider one of:
- Setting
PYTHONUNBUFFERED=1in the workflow env.- Re-opening stdout with
sys.stdout.reconfigure(line_buffering=True)(Python 3.7+).- Calling
flush()from a custom handler after each emit (or viaStreamHandler.terminatorpatterns).🤖 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/core/config.py` at line 44, The call to sys.stdout.flush() immediately after logging.basicConfig() in src.core.config.py is a no-op; remove the flush and instead ensure real-time log output by either setting PYTHONUNBUFFERED=1 in the runtime environment or reconfiguring stdout for line buffering (use sys.stdout.reconfigure(line_buffering=True) on Python 3.7+) after logging.basicConfig(), or implement a StreamHandler that flushes on each emit—update the code around logging.basicConfig and the sys.stdout.flush() call accordingly.src/core/github/projects.py (1)
103-103: ⚡ Quick winUse lazy
%formatting in thislogging.warningcall.This line is touched in the diff but still uses an f-string for the log message, which conflicts with the project-wide rule. Other places in this PR (e.g., Line 60, Lines 283–287) were converted; align this one too.
♻️ Proposed change
- logging.warning(f"GraphQL query for project #{project_number} in org '{org}' failed") + logging.warning("GraphQL query for project #%d in org '%s' failed", project_number, org)As per coding guidelines: "Use lazy % formatting in logging statements:
logger.info(\"msg %s\", var)".🤖 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/core/github/projects.py` at line 103, Replace the f-string in the logging call so it uses lazy % formatting: change the logging.warning invocation that references project_number and org (the call currently using f"GraphQL query for project #{project_number} in org '{org}' failed") to logging.warning("GraphQL query for project #%s in org '%s' failed", project_number, org) so the message follows the project-wide lazy logging convention.src/security/constants.py (1)
32-33: ⚡ Quick winConsider a LoggerAdapter or helper instead of raw prefix constants.
Every call site now does
LOGGING_PREFIX + "msg %s". That works (the concatenation still yields a literal format string, so lazy%-formatting is preserved), but it spreads the prefix convention across many files and is easy to forget — for example, Line 270 insrc/security/collect_alert.pyalready omitsLOGGING_PREFIXwhile neighboring lines use it.A
logging.LoggerAdapter(or a thinget_security_logger(dry_run: bool)factory) would centralize prefix selection (live vs. dry-run) and make it impossible to forget at a call site.🤖 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/constants.py` around lines 32 - 33, Replace the raw LOGGING_PREFIX and DRY_RUN_PREFIX string constants with a centralized logger helper: implement a get_security_logger(dry_run: bool) factory that returns a logging.Logger or logging.LoggerAdapter which automatically prepends the appropriate prefix, and update call sites (e.g., uses in collect_alert.py and other modules referencing LOGGING_PREFIX/DRY_RUN_PREFIX) to call get_security_logger(dry_run) and use logger.info/error/etc. without manual prefix concatenation; ensure the adapter preserves lazy formatting and that tests or callers that previously relied on the constants now import and use get_security_logger (or the LoggerAdapter class) instead.src/security/main.py (1)
180-180: 💤 Low valueUse the module logger for consistency.
The rest of this file uses the module-level
logger = logging.getLogger(__name__), but the closingProcess finishedmessage uses the rootloggingAPI. Switching keeps the logger name consistent with other records emitted by this module.🔧 Proposed change
- logging.info(LOGGING_PREFIX + "Process finished") + logger.info(LOGGING_PREFIX + "Process finished")🤖 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/main.py` at line 180, The final log call uses the root logging API instead of the module-level logger; replace the call that uses logging.info(...) with the module logger (logger.info(...)) so the "Process finished" message is emitted with the same logger name as the rest of src/security/main.py; locate the standalone logging.info(LOGGING_PREFIX + "Process finished") and switch it to use logger.info(LOGGING_PREFIX + "Process finished").tests/security/issues/test_sync.py (1)
821-880: ⚡ Quick winRename
test_label_orphan_*to mirror the helper rename.The section banner at line 817 was updated to
_label_adept_to_close_issues, but the six test functions below still carry the legacytest_label_orphan_*names (and docstrings/comments referencing "orphan"). After the helper rename, these names create a discoverability gap — searching forlabel_adept_to_closewon’t surface the tests, and the suite reads as if it’s testing something that no longer exists.♻️ Suggested rename
-def test_label_orphan_no_orphans() -> None: +def test_label_adept_to_close_no_unmatched() -> None: """No labelling when all children have matching alerts.""" @@ -def test_label_orphan_found(mocker: MockerFixture) -> None: +def test_label_adept_to_close_found(mocker: MockerFixture) -> None: """Labels child issues that have no matching alert.""" @@ -def test_label_orphan_dry_run() -> None: +def test_label_adept_to_close_dry_run() -> None: """Dry-run: logs but does not call gh.""" @@ -def test_label_orphan_skips_already_labelled() -> None: +def test_label_adept_to_close_skips_already_labelled() -> None: """Skips if adept-to-close label is already present.""" @@ -def test_label_orphan_skips_closed_issues() -> None: +def test_label_adept_to_close_skips_closed_issues() -> None: """Closed child issues are not labelled as unmatched.""" @@ -def test_label_orphan_no_repo_in_secmeta() -> None: +def test_label_adept_to_close_no_repo_in_secmeta() -> None: """Skips labelling if no repo in secmeta."""🤖 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/issues/test_sync.py` around lines 821 - 880, Rename the six test functions and their docstrings/comments from the legacy test_label_orphan_* names to names that reference the new helper _label_adept_to_close_issues (e.g., test_label_adept_to_close_no_orphans, test_label_adept_to_close_found, test_label_adept_to_close_dry_run, test_label_adept_to_close_skips_already_labelled, test_label_adept_to_close_skips_closed_issues, test_label_adept_to_close_no_repo_in_secmeta) and update any occurrences of "orphan" in their docstrings to mention "adept_to_close" or "unmatched alert" so tests are discoverable and descriptions match the helper name (_label_adept_to_close_issues) used in the diff.
🤖 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 `@src/core/github/projects.py`:
- Line 27: This imports DRY_RUN_PREFIX from security into core (layering
violation); remove the import and instead add a constructor parameter (e.g.,
prefix or logging_prefix) to ProjectPrioritySync and store it as an instance
attribute, update all uses inside ProjectPrioritySync to reference self.prefix,
and update all callers (including security package) to pass
security.constants.DRY_RUN_PREFIX when constructing ProjectPrioritySync;
alternatively, if you prefer the other option, move DRY_RUN_PREFIX into core
(e.g., core/config or core/logging_prefix) and re-export from security, but do
not keep the direct import from security in src/core/github/projects.py.
In `@src/security/collect_alert.py`:
- Line 234: The log message using logger.info(LOGGING_PREFIX + "Success fetching
repository metadata") is unnatural; update it to past-tense natural English
(e.g., use "Successfully fetched repository metadata") by changing the string
concatenated with LOGGING_PREFIX where logger.info is called so the message
reads correctly.
- Around line 269-270: Change the two logging calls to use the LOGGING_PREFIX on
both lines, use %d (not %i) for the numeric placeholder, and fix the
wording/pluralization—e.g., in the logger.info that references logger,
LOGGING_PREFIX, count and state replace "Successfully fetched %i %s security
alert/s" with a message that uses "%d" and a clean plural form like
"Successfully fetched %d %s security alert(s)" (or compute pluralization from
count if preferred), and in the logger.debug that references out_file and
LOGGING_PREFIX reword the tense to something like "Saved fetched security alerts
to: %s" while prefixing it with LOGGING_PREFIX so both logs are consistent.
In `@src/security/issues/sync.py`:
- Around line 879-916: The indented "Parent issues" and "Child issues" lines in
_log_sync_summary currently hardcode the "[DRY-RUN]" label; update those lines
to use the correct prefix determined earlier (DRY_RUN_PREFIX vs LOGGING_PREFIX)
or compute a small tag variable (e.g., tag = DRY_RUN_PREFIX if dry_run else "")
and prepend that tag instead of the literal "[DRY-RUN]". Ensure you adjust the
string concatenation for both the parent and child lines (refer to
_log_sync_summary, parent_parts, child_parts, and prefix) so non-dry runs do not
display the dry-run label.
In `@src/security/main.py`:
- Around line 135-138: The start-process log statements use eager "%"
interpolation; update both branches so logger.info receives the format string
and repo as a separate argument (e.g., logger.info(LOGGING_PREFIX + "Starting
the DRY-RUN process for %s", repo) and logger.info(LOGGING_PREFIX + "Starting
process for %s", repo)) to enable lazy formatting; change the two calls around
the dry_run check that reference logger, LOGGING_PREFIX, and repo accordingly.
---
Outside diff comments:
In `@docs/security/aquasec-night-scan-example.yml`:
- Around line 25-46: The pinned commit SHA for the reusable workflow in
jobs.scan
(AbsaOSS/organizational-workflows/.github/workflows/aquasec-scan.yml@c1fa5b54ff24fea071415da89abc4f0506344f01)
points to a revision that does not declare the dry-run input; update that SHA to
a commit where the reusable workflow aquasec-scan.yml includes an inputs.dry-run
definition so the with: dry-run: ${{ inputs.dry-run || false }} call is valid,
ensuring the jobs.scan uses the new commit hash that exposes the dry-run input.
In `@src/security/issues/sync.py`:
- Around line 411-441: The live-branch is calling
gh_issue_add_sub_issue_by_number(...) to link a newly created child to a parent
but does not increment sync.stats.children_linked, causing the counter to be
understated; update the block in the new-child path (after
maybe_reopen_parent_issue(...) and after gh_issue_add_sub_issue_by_number(...))
to increment sync.stats.children_linked on successful linking (matching the
dry-run behavior and the pattern used elsewhere, e.g., line with
sync.stats.children_linked increment at the dry-run branch and at line 629),
ensuring the increment occurs only in non-dry-run/live execution and after the
gh_issue_add_sub_issue_by_number call succeeds.
---
Nitpick comments:
In `@src/core/config.py`:
- Line 44: The call to sys.stdout.flush() immediately after
logging.basicConfig() in src.core.config.py is a no-op; remove the flush and
instead ensure real-time log output by either setting PYTHONUNBUFFERED=1 in the
runtime environment or reconfiguring stdout for line buffering (use
sys.stdout.reconfigure(line_buffering=True) on Python 3.7+) after
logging.basicConfig(), or implement a StreamHandler that flushes on each
emit—update the code around logging.basicConfig and the sys.stdout.flush() call
accordingly.
In `@src/core/github/projects.py`:
- Line 103: Replace the f-string in the logging call so it uses lazy %
formatting: change the logging.warning invocation that references project_number
and org (the call currently using f"GraphQL query for project #{project_number}
in org '{org}' failed") to logging.warning("GraphQL query for project #%s in org
'%s' failed", project_number, org) so the message follows the project-wide lazy
logging convention.
In `@src/security/constants.py`:
- Around line 32-33: Replace the raw LOGGING_PREFIX and DRY_RUN_PREFIX string
constants with a centralized logger helper: implement a
get_security_logger(dry_run: bool) factory that returns a logging.Logger or
logging.LoggerAdapter which automatically prepends the appropriate prefix, and
update call sites (e.g., uses in collect_alert.py and other modules referencing
LOGGING_PREFIX/DRY_RUN_PREFIX) to call get_security_logger(dry_run) and use
logger.info/error/etc. without manual prefix concatenation; ensure the adapter
preserves lazy formatting and that tests or callers that previously relied on
the constants now import and use get_security_logger (or the LoggerAdapter
class) instead.
In `@src/security/main.py`:
- Line 180: The final log call uses the root logging API instead of the
module-level logger; replace the call that uses logging.info(...) with the
module logger (logger.info(...)) so the "Process finished" message is emitted
with the same logger name as the rest of src/security/main.py; locate the
standalone logging.info(LOGGING_PREFIX + "Process finished") and switch it to
use logger.info(LOGGING_PREFIX + "Process finished").
In `@tests/security/issues/test_sync.py`:
- Around line 821-880: Rename the six test functions and their
docstrings/comments from the legacy test_label_orphan_* names to names that
reference the new helper _label_adept_to_close_issues (e.g.,
test_label_adept_to_close_no_orphans, test_label_adept_to_close_found,
test_label_adept_to_close_dry_run,
test_label_adept_to_close_skips_already_labelled,
test_label_adept_to_close_skips_closed_issues,
test_label_adept_to_close_no_repo_in_secmeta) and update any occurrences of
"orphan" in their docstrings to mention "adept_to_close" or "unmatched alert" so
tests are discoverable and descriptions match the helper name
(_label_adept_to_close_issues) used in the diff.
🪄 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: ec14afdd-9bbd-416c-ad8e-e38f2a81701c
📒 Files selected for processing (18)
.github/copilot-instructions.md.github/workflows/aquasec-night-scan.yml.github/workflows/aquasec-scan.ymldocs/security/aquasec-night-scan-example.ymlsrc/core/config.pysrc/core/github/issues.pysrc/core/github/projects.pysrc/security/alerts/parser.pysrc/security/collect_alert.pysrc/security/constants.pysrc/security/issues/models.pysrc/security/issues/sync.pysrc/security/main.pysrc/security/notifications/teams.pysrc/security/promote_alerts.pytests/security/issues/test_sync.pytests/security/notifications/test_teams.pytests/security/test_main.py
💤 Files with no reviewable changes (1)
- tests/security/notifications/test_teams.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/security/issues/sync.py (1)
411-442:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
children_linkedis undercounted in non-dry-run mode for newly created children.In dry-run with a parent issue, line 382 increments
sync.stats.children_linked. However, in the non-dry-run path (line 442),gh_issue_add_sub_issue_by_numberis called for the new child butsync.stats.children_linkedis not incremented. This causes a real-vs-dry-run stats divergence: a dry-run will report N linked children, while the actual run will report fewer (only the increments from_ensure_child_linked_to_parentfor existing children).🔧 Proposed fix
if parent_issue is not None: maybe_reopen_parent_issue( ctx.repo, parent_issue, rule_id=ctx.rule_id, dry_run=sync.dry_run, context="new_child", child_issue_number=num, ) logging.debug("Add sub-issue link parent=#%d child=#%d (alert %d)", parent_issue.number, num, ctx.alert_number) - gh_issue_add_sub_issue_by_number(ctx.repo, parent_issue.number, num) + if gh_issue_add_sub_issue_by_number(ctx.repo, parent_issue.number, num): + sync.stats.children_linked += 1🤖 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/issues/sync.py` around lines 411 - 442, The non-dry-run path creating a new child calls gh_issue_add_sub_issue_by_number but never increments sync.stats.children_linked, causing stat divergence; update the block in sync.py that handles a newly created child (after maybe_reopen_parent_issue and/or immediately after gh_issue_add_sub_issue_by_number) to increment sync.stats.children_linked by 1 when a link is actually created (mirror the dry-run increment), ensuring the counter and logging reflect the real linked-child action.
🧹 Nitpick comments (1)
tests/security/issues/test_sync.py (1)
557-566: ⚡ Quick winAdd a parallel non-dry-run test for
children_linkedstats with a parent.This test validates the dry-run path increments both
children_createdandchildren_linkedwhen a parent is provided. There is no equivalent live-path test, which is why the missing increment in the non-dry-run branch (see comment onsrc/security/issues/sync.pylines 411–442) went undetected. Adding a symmetric test will lock in the corrected behavior.🔧 Suggested addition
def test_handle_new_child_real_with_parent_increments_linked(mocker: MockerFixture, sast_alert: Alert) -> None: """Non-dry-run with a parent increments children_linked alongside children_created.""" mocker.patch("security.issues.sync.gh_issue_create", return_value=42) mocker.patch("security.issues.sync.gh_issue_add_sub_issue_by_number", return_value=True) parent = Issue(number=7, state="open", title="P", body="pb") ctx = _make_alert_context(alert=sast_alert) sync = _make_sync_context(notifications=[]) _handle_new_child_issue(ctx=ctx, sync=sync, parent_issue=parent) assert 1 == sync.stats.children_created assert 1 == sync.stats.children_linked🤖 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/issues/test_sync.py` around lines 557 - 566, Add a non-dry-run counterpart to the existing dry-run test to ensure the live-path increments children_linked when a parent is provided: create a new test (e.g., test_handle_new_child_real_with_parent_increments_linked) that patches security.issues.sync.gh_issue_create and gh_issue_add_sub_issue_by_number to return success, constructs a parent Issue and a real (non-dry-run) sync context via _make_sync_context, calls _handle_new_child_issue(ctx=..., sync=..., parent_issue=parent) and asserts sync.stats.children_created == 1 and sync.stats.children_linked == 1 so the non-dry-run branch behavior is verified.
🤖 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 `@src/security/issues/sync.py`:
- Line 391: Replace the non-ASCII punctuation in the log messages: change the EN
DASH character (U+2013) used in the logging.debug message that contains "No
parent issue yet for rule_id=%s – link will happen on next sync" (and the two
other flagged logging.debug messages referencing ctx.rule_id) to a plain ASCII
hyphen-minus '-' and replace the ellipsis character (U+2026) found in the other
logged message with three ASCII dots "..." so all logging strings use only plain
ASCII punctuation; update the three logging.debug messages (the ones using
logging.debug and ctx.rule_id) accordingly.
- Around line 583-592: The counter sync.stats.children_title_updated is being
incremented regardless of whether the title edit actually succeeded; move the
increment so it only happens when an update occurs (mirror the parent-title
handling). Specifically, inside the else branch where
gh_issue_edit_title(ctx.repo, issue.number, expected_title) is called, increment
sync.stats.children_title_updated only after gh_issue_edit_title returns True
and you set issue.title = expected_title and log the success; do not increment
it in the outer expected_title != (issue.title or "") scope or in the dry-run
branch.
In `@src/security/main.py`:
- Around line 153-155: The debug log in the overwrite branch of the output-file
handling omits the required domain prefix; update the logger.debug call in the
block that checks os.path.exists(out_file) (around the code using out_file and
logger) to prepend LOGGING_PREFIX (e.g., LOGGING_PREFIX + " - ") to the message
so it follows the "Security - ..." convention; ensure the message content
remains "Output file is already present: overwriting" but begins with
LOGGING_PREFIX.
---
Outside diff comments:
In `@src/security/issues/sync.py`:
- Around line 411-442: The non-dry-run path creating a new child calls
gh_issue_add_sub_issue_by_number but never increments
sync.stats.children_linked, causing stat divergence; update the block in sync.py
that handles a newly created child (after maybe_reopen_parent_issue and/or
immediately after gh_issue_add_sub_issue_by_number) to increment
sync.stats.children_linked by 1 when a link is actually created (mirror the
dry-run increment), ensuring the counter and logging reflect the real
linked-child action.
---
Nitpick comments:
In `@tests/security/issues/test_sync.py`:
- Around line 557-566: Add a non-dry-run counterpart to the existing dry-run
test to ensure the live-path increments children_linked when a parent is
provided: create a new test (e.g.,
test_handle_new_child_real_with_parent_increments_linked) that patches
security.issues.sync.gh_issue_create and gh_issue_add_sub_issue_by_number to
return success, constructs a parent Issue and a real (non-dry-run) sync context
via _make_sync_context, calls _handle_new_child_issue(ctx=..., sync=...,
parent_issue=parent) and asserts sync.stats.children_created == 1 and
sync.stats.children_linked == 1 so the non-dry-run branch behavior is verified.
🪄 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: d65f9a8e-4837-4f10-94a6-d01ed7acc9ae
📒 Files selected for processing (4)
src/security/collect_alert.pysrc/security/issues/sync.pysrc/security/main.pytests/security/issues/test_sync.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/security/issues/test_sync.py (1)
1117-1140: ⚡ Quick winHardcode-coupling on prefix strings will silently break tests when constants change.
The parametrize values on lines 1118–1119 duplicate the literal contents of
LOGGING_PREFIXandDRY_RUN_PREFIXfromsecurity.constants. If those constants are ever tweaked (rename, spacing, decoration), these tests will fail in a confusing way without protecting any real behavior. Importing the constants directly couples the tests to the production source of truth.♻️ Proposed refactor
+from security.constants import DRY_RUN_PREFIX, LOGGING_PREFIX @@ -@pytest.mark.parametrize("dry_run,prefix", [ - (False, "Security Alerts to Issues - "), - (True, "Security Alerts to Issues [DRY-RUN] - "), -]) +@pytest.mark.parametrize("dry_run,prefix", [ + (False, LOGGING_PREFIX), + (True, DRY_RUN_PREFIX), +])🤖 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/issues/test_sync.py` around lines 1117 - 1140, Replace the hardcoded prefix strings in test_log_sync_summary with the canonical constants from security.constants: import LOGGING_PREFIX and DRY_RUN_PREFIX and change the parametrize tuple to use (False, LOGGING_PREFIX) and (True, DRY_RUN_PREFIX); update references in the assertions to use the injected prefix variable so the test adapts if LOGGING_PREFIX/DRY_RUN_PREFIX change and still validates _log_sync_summary behavior.
🤖 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 `@src/security/issues/sync.py`:
- Around line 560-571: _in _rebuild_and_apply_child_body and
_flush_parent_body_updates the counters children_body_updated and
parents_body_updated are being incremented unconditionally even when a live
gh_issue_edit_body/gh_issue_edit_title call may have failed; change the live
branch to capture and check the return value from gh_issue_edit_body (call it
and store its result) and only increment sync.stats.children_body_updated (and
similarly sync.stats.parents_body_updated in _flush_parent_body_updates) when
that result indicates success, while leaving the unconditional increment in the
sync.dry_run path so dry-run still reports updates.
---
Nitpick comments:
In `@tests/security/issues/test_sync.py`:
- Around line 1117-1140: Replace the hardcoded prefix strings in
test_log_sync_summary with the canonical constants from security.constants:
import LOGGING_PREFIX and DRY_RUN_PREFIX and change the parametrize tuple to use
(False, LOGGING_PREFIX) and (True, DRY_RUN_PREFIX); update references in the
assertions to use the injected prefix variable so the test adapts if
LOGGING_PREFIX/DRY_RUN_PREFIX change and still validates _log_sync_summary
behavior.
🪄 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: 60021d5c-fa21-47aa-b70b-164c2efdc2ae
📒 Files selected for processing (3)
src/security/issues/models.pysrc/security/issues/sync.pytests/security/issues/test_sync.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/security/issues/sync.py (1)
432-441:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate child-to-parent link reporting on the actual link result.
The new-child path fires
gh_issue_add_sub_issue_by_number(...)without checking it, so live runs never incrementchildren_linkedthere. The repair path does check the helper, but it logs a successful link before that check. Together, that makes the new sync summary undercount links and can claim a link succeeded when GitHub rejected it.Suggested minimal fix
if parent_issue is not None: maybe_reopen_parent_issue( ctx.repo, parent_issue, rule_id=ctx.rule_id, dry_run=sync.dry_run, stats=sync.stats, ) - logging.debug("Add sub-issue link parent=#%d child=#%d (alert %d)", parent_issue.number, num, ctx.alert_number) - gh_issue_add_sub_issue_by_number(ctx.repo, parent_issue.number, num) + logging.debug( + LOGGING_PREFIX + "Adding sub-issue link parent=#%d child=#%d (alert %d)", + parent_issue.number, + num, + ctx.alert_number, + ) + if gh_issue_add_sub_issue_by_number(ctx.repo, parent_issue.number, num): + logging.info(LOGGING_PREFIX + "Linked child issue #%d to parent #%d", num, parent_issue.number) + sync.stats.children_linked += 1 ... - logging.info( - LOGGING_PREFIX + "Linked child issue #%d to parent #%d", - issue.number, - parent_issue.number, - ) if gh_issue_add_sub_issue_by_number(ctx.repo, parent_issue.number, issue.number): + logging.info( + LOGGING_PREFIX + "Linked child issue #%d to parent #%d", + issue.number, + parent_issue.number, + ) cache[parent_issue.number].add(issue.number) sync.stats.children_linked += 1Also applies to: 627-634
🤖 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/issues/sync.py` around lines 432 - 441, The code logs and counts a successful child→parent link before verifying gh_issue_add_sub_issue_by_number actually succeeded; change both the "new-child" and the "repair" paths so you call gh_issue_add_sub_issue_by_number(...) and check its return value (or catch its failure) before logging success and incrementing sync.stats.children_linked, and only log an error/skip increment when the helper indicates the link failed; update the blocks around maybe_reopen_parent_issue and gh_issue_add_sub_issue_by_number (also mirror the same fix in the similar block around lines handling the repair path mentioned) so the logging and stats update occur strictly after a successful helper result.
🤖 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 `@src/security/issues/sync.py`:
- Line 278: The new debug logs in src/security/issues/sync.py use raw "DRY-RUN:"
or missing domain prefixes; update those logging.debug calls (the ones
referencing existing.number / expected_title and the other occurrences around
lines 302-304, 395-397, 564-566, 586-592, 774-776) to prepend the standard
domain prefix "Security - " (e.g., "Security - DRY-RUN: ...") so all messages
follow the "Security - <message>" format; modify each logging.debug invocation
accordingly while preserving existing interpolation of variables like
existing.number and expected_title.
- Around line 821-832: The code logs "Marked issue ..." and increments
stats.children_marked_for_closure before confirming the label mutation
succeeded; change the flow so that for dry_run you keep the existing DRY_RUN log
only, but in the live path call gh_issue_add_labels(repo, issue.number,
[LABEL_SEC_ADEPT_TO_CLOSE]) first (wrap it to catch/handle exceptions or check
its success), and only after it returns successfully emit the LOGGING_PREFIX
"Marked issue #%d for closure..." message and increment
stats.children_marked_for_closure; on mutation failure log an error and do not
increment the stat. Ensure you reference gh_issue_add_labels,
stats.children_marked_for_closure, LABEL_SEC_ADEPT_TO_CLOSE, and the dry_run
branch when making this change.
---
Outside diff comments:
In `@src/security/issues/sync.py`:
- Around line 432-441: The code logs and counts a successful child→parent link
before verifying gh_issue_add_sub_issue_by_number actually succeeded; change
both the "new-child" and the "repair" paths so you call
gh_issue_add_sub_issue_by_number(...) and check its return value (or catch its
failure) before logging success and incrementing sync.stats.children_linked, and
only log an error/skip increment when the helper indicates the link failed;
update the blocks around maybe_reopen_parent_issue and
gh_issue_add_sub_issue_by_number (also mirror the same fix in the similar block
around lines handling the repair path mentioned) so the logging and stats update
occur strictly after a successful helper result.
🪄 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: ca8d87cd-b70a-40b5-b614-8140783af59e
📒 Files selected for processing (1)
src/security/issues/sync.py
miroslavpojer
approved these changes
May 7, 2026
Contributor
miroslavpojer
left a comment
miroslavpojer
left a comment
There was a problem hiding this comment.
I do not see issues.
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
3 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 introduces a "dry-run" capability to the AquaSec security alert-to-issue workflow, improves logging consistency, and adds better tracking of sync actions. The dry-run mode allows the workflow to simulate actions without making persistent changes, which is useful for testing and validation. Additionally, logging output is standardized and made more informative, and internal statistics are now tracked for key sync operations.
Release Notes
Related
Closes #22
Summary by CodeRabbit
New Features
Improvements
Documentation