Remove obsolete parts of the PoC#41
Merged
tmikula-dev merged 8 commits intomasterfrom Apr 24, 2026
Merged
Conversation
tmikula-dev
added
enhancement
WalkthroughThis PR removes obsolete infrastructure from a security issue tracking system. It deletes two deprecated extraction/metrics modules, removes sec-event comment and block handling functionality, simplifies GitHub client environment variable passing, adds a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 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)
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 |
Closed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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)
101-126:⚠️ Potential issue | 🟡 MinorStale docstring — sec-event comment recording was removed.
The docstring still says "and record a sec-event comment," but per this PR the sec-event commenting side-effect was deleted. Update to reflect that the function only transitions state.
📝 Suggested fix
def maybe_reopen_parent_issue( repo: str, parent_issue: Issue | None, *, rule_id: str, dry_run: bool, context: str, child_issue_number: int | None = None, ) -> None: - """Reopen *parent_issue* (if closed) and record a sec-event comment.""" + """Reopen *parent_issue* (if closed)."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/security/issues/sync.py` around lines 101 - 126, The docstring for maybe_reopen_parent_issue is stale (it says it "and record a sec-event comment") but the function no longer posts comments; update the docstring to accurately describe current behavior: that it will reopen the provided parent_issue if it is closed (and only logs/sets state, respecting dry_run), and remove any mention of recording sec-event comments; locate maybe_reopen_parent_issue and adjust its triple-quoted description to reflect the state-transition-only behavior.
🧹 Nitpick comments (2)
tests/security/issues/test_builder.py (2)
284-286: Consider tightening the fallback-to-rule_name assertion.
"sast" in titleis satisfied by any occurrence; an exact-match or prefix check would better validate that the summary is actually the rule_name fallback.♻️ Suggested fix
def test_fallback_to_rule_name() -> None: title = build_issue_title(None, "sast", "rule-123", "abcdef12") - assert "sast" in title + assert title == "[SEC][FP=abcdef12] sast"🤖 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 284 - 286, The test test_fallback_to_rule_name currently asserts a loose substring match ("sast" in title); tighten it to ensure the title actually uses the rule_name fallback by asserting a precise relation (for example use title.startswith("sast") or assert title == "sast") when calling build_issue_title(None, "sast", "rule-123", "abcdef12"); update the assertion accordingly to replace the substring check with a prefix or exact-match assertion on the generated title.
308-318:test_sast_avd_idduplicatestest_sast_titleand no longer tests the AVD ID field.Both tests now assert the same substring (
"Requests with verify=False" in body). Since the SAST fixture'svulnerabilitydoes not start with"AVD-", the rendered AVD ID line is"N/A", not the rule description — the assertion here passes only because the string appears in the**Title:**line. Either tighten this test to check the AVD ID line specifically (e.g.,N/Afor SAST), or remove it to avoid redundancy.♻️ Suggested fix
def test_sast_avd_id(sast_alert: Alert) -> None: body = build_child_issue_body(sast_alert) - assert "Requests with verify=False" in body + # SAST vulnerability is not an "AVD-*" code, so AVD ID renders as N/A + assert "**AVD ID:** N/A" in body🤖 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 308 - 318, The test_sast_avd_id test duplicates test_sast_title by asserting the rule description appears in the body; instead update test_sast_avd_id to specifically assert the AVD ID line produced by build_child_issue_body for SAST alerts is "N/A" (because the SAST fixture's vulnerability does not start with "AVD-"), e.g. check that the AVD ID field in the rendered body equals or contains "AVD ID: N/A"; alternatively, if you prefer to keep fewer tests, remove test_sast_avd_id to avoid redundancy and rely on test_sast_title and test_sast_alert_hash for coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/security/issues/test_secmeta.py`:
- Around line 118-121: The generator expressions that find type_idx and fp_idx
use an ambiguous loop variable named "l"; rename that variable to "line" in both
occurrences (the expressions computing type_idx and fp_idx that iterate over
"lines") so the test in test_secmeta.py uses clear, lint-clean variable names
while preserving behavior.
---
Outside diff comments:
In `@src/security/issues/sync.py`:
- Around line 101-126: The docstring for maybe_reopen_parent_issue is stale (it
says it "and record a sec-event comment") but the function no longer posts
comments; update the docstring to accurately describe current behavior: that it
will reopen the provided parent_issue if it is closed (and only logs/sets state,
respecting dry_run), and remove any mention of recording sec-event comments;
locate maybe_reopen_parent_issue and adjust its triple-quoted description to
reflect the state-transition-only behavior.
---
Nitpick comments:
In `@tests/security/issues/test_builder.py`:
- Around line 284-286: The test test_fallback_to_rule_name currently asserts a
loose substring match ("sast" in title); tighten it to ensure the title actually
uses the rule_name fallback by asserting a precise relation (for example use
title.startswith("sast") or assert title == "sast") when calling
build_issue_title(None, "sast", "rule-123", "abcdef12"); update the assertion
accordingly to replace the substring check with a prefix or exact-match
assertion on the generated title.
- Around line 308-318: The test_sast_avd_id test duplicates test_sast_title by
asserting the rule description appears in the body; instead update
test_sast_avd_id to specifically assert the AVD ID line produced by
build_child_issue_body for SAST alerts is "N/A" (because the SAST fixture's
vulnerability does not start with "AVD-"), e.g. check that the AVD ID field in
the rendered body equals or contains "AVD ID: N/A"; alternatively, if you prefer
to keep fewer tests, remove test_sast_avd_id to avoid redundancy and rely on
test_sast_title and test_sast_alert_hash for coverage.
🪄 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: afae2846-ab65-4595-9b8b-66a12b6d58c8
📒 Files selected for processing (25)
.github/workflows/remove-adept-to-close-on-issue-close.ymlpyproject.tomlsrc/core/github/client.pysrc/core/github/projects.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/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 (8)
- tests/security/test_constants.py
- src/security/issues/templates.py
- src/security/issues/sec_events.py
- .github/workflows/remove-adept-to-close-on-issue-close.yml
- src/security/extract_team_security_stats.py
- src/security/constants.py
- src/security/derive_team_security_metrics.py
- tests/security/issues/test_sec_events.py
Comment on lines
+118
to
+121
| # type should appear before fingerprint per preferred_order | ||
| 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) | ||
| assert schema_idx < fp_idx | ||
| assert type_idx < fp_idx |
There was a problem hiding this comment.
Rename the ambiguous generator variable.
Ruff flags l as ambiguous here; use line to keep the test lint-clean.
Proposed fix
- 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)🧰 Tools
🪛 Ruff (0.15.10)
[error] 119-119: Ambiguous variable name: l
(E741)
[error] 120-120: Ambiguous variable name: l
(E741)
🤖 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 118 - 121, The generator
expressions that find type_idx and fp_idx use an ambiguous loop variable named
"l"; rename that variable to "line" in both occurrences (the expressions
computing type_idx and fp_idx that iterate over "lines") so the test in
test_secmeta.py uses clear, lint-clean variable names while preserving behavior.
# Conflicts: # .github/workflows/remove-adept-to-close-on-issue-close.yml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/aquasec-scan.yml (1)
108-114:⚠️ Potential issue | 🟠 MajorDo not ship the reusable workflow pinned to a feature branch.
Line 112 checks out mutable feature branch helper code. After merge, this breaks consumers if the branch is deleted or diverges; while it exists, any update to the branch changes code running with
issues: write/security-events: writepermissions. Pin to an immutable ref—eithergithub.workflow_sha(matching the workflow revision) or a stable release tag likev0.6.0.🔒 Proposed fix
- name: Checkout code uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd with: repository: AbsaOSS/organizational-workflows - ref: feature/remove-obsolete-parts + ref: ${{ github.workflow_sha }} path: org-workflows persist-credentials: falseAlternatively, use a stable release tag (e.g.,
v0.6.0) if the intent is always "latest production helper code" rather than "same revision as this reusable workflow."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/aquasec-scan.yml around lines 108 - 114, The workflow currently checks out a mutable feature branch in the reusable workflow step (the actions/checkout invocation that sets ref: feature/remove-obsolete-parts); change that ref to an immutable identifier so consumers aren't broken by branch changes—replace the ref with the workflow-scoped immutable SHA (use ${{ github.workflow_sha }}) or a stable release tag (e.g., v0.6.0) before shipping the reusable workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/aquasec-scan.yml:
- Around line 108-114: The workflow currently checks out a mutable feature
branch in the reusable workflow step (the actions/checkout invocation that sets
ref: feature/remove-obsolete-parts); change that ref to an immutable identifier
so consumers aren't broken by branch changes—replace the ref with the
workflow-scoped immutable SHA (use ${{ github.workflow_sha }}) or a stable
release tag (e.g., v0.6.0) before shipping the reusable workflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8cecae10-3256-477c-86da-8763e68374c5
📒 Files selected for processing (1)
.github/workflows/aquasec-scan.yml
Collaborator
Author
|
I did testing in the repository: https://github.com/AbsaOSS/living-doc-generator-mdoc. In the following links you can see the new generated epic and child with new metadata.
The solution also fix renaming the generic name from vulnerabilities into specific child title. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/core/github/test_issues.py (1)
333-344: Consider adding parity coverage for the warning/not-found branch.
gh_issue_add_labelshas three tests (success, empty, not-found hint) butgh_issue_remove_labelsonly has two. The warning path at line 295 ofsrc/core/github/issues.pyis currently uncovered.🧪 Suggested additional test
def test_remove_labels_no_labels_skips_call(mocker: MockerFixture) -> None: mock_run = mocker.patch("core.github.issues.run_gh") gh_issue_remove_labels("org/repo", 1, []) mock_run.assert_not_called() + +def test_remove_labels_not_found_hint(mocker: MockerFixture, caplog) -> None: + mocker.patch("core.github.issues.run_gh", return_value=_not_found()) + with caplog.at_level(logging.WARNING, logger="root"): + gh_issue_remove_labels("org/repo", 1, ["sec:adept-to-close"]) + assert any("deleted or transferred" in r.message for r in caplog.records)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/github/test_issues.py` around lines 333 - 344, Add a test that covers the "not-found/warning" branch of gh_issue_remove_labels by mocking core.github.issues.run_gh to return the not-found response (the same shape used by the gh_issue_add_labels not-found test) and asserting that run_gh is called and the code logs or handles the warning path; specifically add a new test in tests/core/github/test_issues.py that calls gh_issue_remove_labels("org/repo", 1, ["label"]) with run_gh patched to return the _not_found() fixture and then asserts the warning behavior (e.g., mock for logger.warning or expected side-effect), referencing gh_issue_remove_labels and run_gh to locate the code.
🤖 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/sync.py`:
- Around line 444-463: The in-memory label is removed unconditionally even if
the GH API call fails; change _remove_adept_to_close_label to only mutate
issue.labels when the remote removal succeeds: update gh_issue_remove_labels to
return a boolean success indicator (or raise on failure) and in
_remove_adept_to_close_label call it, check the returned bool (or catch the
exception) and only call issue.labels.remove(LABEL_SEC_ADEPT_TO_CLOSE) when the
call succeeded; keep the dry_run branch unchanged.
---
Nitpick comments:
In `@tests/core/github/test_issues.py`:
- Around line 333-344: Add a test that covers the "not-found/warning" branch of
gh_issue_remove_labels by mocking core.github.issues.run_gh to return the
not-found response (the same shape used by the gh_issue_add_labels not-found
test) and asserting that run_gh is called and the code logs or handles the
warning path; specifically add a new test in tests/core/github/test_issues.py
that calls gh_issue_remove_labels("org/repo", 1, ["label"]) with run_gh patched
to return the _not_found() fixture and then asserts the warning behavior (e.g.,
mock for logger.warning or expected side-effect), referencing
gh_issue_remove_labels and run_gh to locate the code.
🪄 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: 283db6a1-a994-420e-981f-bf9d55ec8202
📒 Files selected for processing (4)
src/core/github/issues.pysrc/security/issues/sync.pytests/core/github/test_issues.pytests/security/issues/test_sync.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/security/issues/test_sync.py
miroslavpojer
approved these changes
Apr 24, 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 removes deprecated scripts and streamlines the codebase, while also enhancing the security alert data model and improving issue creation details. The most significant changes are the removal of legacy Python scripts, simplification of environment and subprocess handling, and the addition of a
rule_descriptionfield to security alert metadata.Release Notes
Related
Closes #32
Summary by CodeRabbit
Release Notes
New Features
Removals
Improvements