Skip to content

crowdstrike: fix logger.warning dict arg and add related_actors deduplication test#6134

Draft
Copilot wants to merge 3 commits intomasterfrom
copilot/fix-logging-behavior-and-add-tests
Draft

crowdstrike: fix logger.warning dict arg and add related_actors deduplication test#6134
Copilot wants to merge 3 commits intomasterfrom
copilot/fix-logging-behavior-and-add-tests

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 31, 2026

Two review fixes for the related_actorsIntrusionSet/targets feature in the CrowdStrike vulnerability builder.

Changes

  • vulnerability/builder.py — Fix logger.warning() call that passed a dict as the second positional arg; Python's logging %-formatter would raise TypeError at runtime if a handler triggered interpolation on a format string with no % placeholders:

    # Before — dict as second arg, raises TypeError on interpolation
    logger.warning("Skipping related actor with missing or empty name", {"actor": actor})
    
    # After — standard %-style lazy interpolation
    logger.warning("Skipping related actor with missing or empty name: %r", actor)
  • tests/vulnerability_related_actors/test_vulnerability_related_actors.py — Add test_duplicate_related_actors_deduplicated: supplies three related_actors entries with the same name, asserts exactly one intrusion-set and one targets relationship are emitted, exercising the seen_names deduplication path that was previously uncovered.

Original prompt

Address review comments in Pull Request #6133:

  1. Logging behavior:
    In external-import/crowdstrike/src/crowdstrike_feeds_connector/vulnerability/builder.py, the logger.warning() call uses a dictionary as a formatting argument, which can raise TypeError. Replace it with standard logging by using the suggested format:
logger.warning("Skipping related actor with missing or empty name: %r", actor)

This change is related to the code at line 226.

  1. Unit Tests for Deduplication:
    Add a unit test to ensure that the deduplication process works correctly. Specifically:
    • Test related_actors field with duplicate entries (same name repeated).
    • Validate that only one intrusion-set and one targets relationship are emitted.

Commit these fixes and ensure the tests pass successfully.

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: Review comments and fix

Assistant: This pull request has two review comments to address:

Comment 1: Logging behavior issue

  • Summary: logger.warning() is used with a dictionary {} as a formatting argument, which could raise TypeError in production environments unless properly formatted.
  • Suggested Fix: Replace this with standard logging:
    logger.warning("Skipping related actor with missing or empty name: %r", actor)
  • Review Link: View Comment

Comment 2: Test case for duplication in related_actors

  • Summary: The deduplication logic (seen_names) for related_actors is not covered by tests.
  • Action Needed: Add a test case where related_actors contains duplicate names. Ensure only one intrusion-set and one targets relationship are emitted.
  • Review Link: View Comment

Would you like me to implement these changes and update the PR?

User: Yes please

Assistant: The requested changes are being implemented in Pull Request #6133. I will update you once the changes are ready or if any further actions are needed.

This pull request was created from Copilot chat.

Copilot AI and others added 2 commits March 31, 2026 22:46
Copilot AI changed the title [WIP] Address review comments in PR #6133 crowdstrike: fix logger.warning dict arg and add related_actors deduplication test Mar 31, 2026
Copilot AI requested a review from CTIBurn0ut March 31, 2026 22:50
@github-actions
Copy link
Copy Markdown

Thank you for your contribution, but we need you to sign your commits. Please see https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

1 similar comment
@github-actions
Copy link
Copy Markdown

Thank you for your contribution, but we need you to sign your commits. Please see https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants