fix: type hinting fixes and additional code checks #4790

traut · 2025-06-11T13:33:21Z

Pull Request

Issue link(s):

Summary - What I changed

adding ruff and pyright checks in CI workflow
making sure pyright has no complains

How To Test

Checklist

Added a label for the type of pr: bug, enhancement, schema, maintenance, Rule: New, Rule: Deprecation, Rule: Tuning, Hunt: New, or Hunt: Tuning so guidelines can be generated
Added the meta:rapid-merge label if planning to merge within 24 hours
Secret and sensitive material has been managed correctly
Automated testing was updated or added to match the most common scenarios
Documentation and comments were added for features that require explanation

Contributor checklist

Have you signed the contributor license agreement?
Have you followed the contributor guidelines?

github-actions · 2025-06-17T16:32:45Z

Enhancement - Guidelines

These guidelines serve as a reminder set of considerations when addressing adding a new schema feature to the code.

Documentation and Context

Describe the feature enhancement in detail (alternative solutions, description of the solution, etc.) if not already documented in an issue.
Include additional context or screenshots.
Ensure the enhancement includes necessary updates to the documentation and versioning.

Code Standards and Practices

Code follows established design patterns within the repo and avoids duplication.
Code changes do not introduce new warnings or errors.
Variables and functions are well-named and descriptive.
Any unnecessary / commented-out code is removed.
Ensure that the code is modular and reusable where applicable.
Check for proper exception handling and messaging.

Testing

New unit tests have been added to cover the enhancement.
Existing unit tests have been updated to reflect the changes.
Provide evidence of testing and validating the enhancement (e.g., test logs, screenshots).
Validate that any rules affected by the enhancement are correctly updated.
Ensure that performance is not negatively impacted by the changes.
Verify that any release artifacts are properly generated and tested.

Additional Schema Related Checks

Mikaayenson · 2025-06-17T20:28:00Z

.github/CODEOWNERS

-detection_rules/etc/*/*           @mikaayenson @eric-forte-elastic @terrancedejesus
+detection_rules/etc/packages.yaml @mikaayenson @eric-forte-elastic @traut
+detection_rules/etc/*.json        @mikaayenson @eric-forte-elastic @traut
+detection_rules/etc/*/*           @mikaayenson @eric-forte-elastic @traut


Suggested change

detection_rules/etc/*/* @mikaayenson @eric-forte-elastic @traut

detection_rules/etc/*/* @mikaayenson @eric-forte-elastic @traut

# exclude files from code owners

detection_rules/etc/non-ecs-schema.json

per our team discussion in the team sync today.

Mikaayenson · 2025-06-17T20:38:41Z

hunting/run.py

            query += " | LIMIT 10"
            click.echo("No LIMIT detected in query. Added LIMIT 10 to truncate output.")
        return query

-    def run_individual_query(self, query: str, wait_timeout: int):
+    def run_individual_query(self, query: str, _: int):


Mikaayenson · 2025-06-17T20:41:32Z

tests/base.py

@@ -65,13 +66,14 @@ def setUpClass(cls):
            except Exception as e:
                RULE_LOADER_FAIL = True
                RULE_LOADER_FAIL_MSG = str(e)
+                raise


Is the message supposed to be rolled up instead of failing here?

Mikaayenson · 2025-06-17T20:47:04Z

tests/test_all_rules.py

-        config = '## Setup\n\n'
-        beats_integration_pattern = config + 'The {} Fleet integration, Filebeat module, or similarly ' \
-                                             'structured data is required to be compatible with this rule.'
+        config = "## Setup\n\n"


Should we just delete this test or is it a bug?

TLDR I think we can delete this test.

I think the intent for this unittest.skip was similar to / the inverse of

@unittest.skipIf(PACKAGE_STACK_VERSION < Version("8.3.0"), "Test only applicable to 8.3+ stacks regarding related integrations build time field.")

Which were both added in 2429 to address

In 8.3, we added new build-time fields to our rules, specifically required_fields,related_integrations,setup. This feature request focuses solely on the related_integrations field.

At this time to determine which integrations to build the integrations manifest file, we rely on the integrations folder to determine this and then reference these names in package-storage. For matching, we rely solely on event.dataset fields in these integration queries

The issue:
We do not include the endpoint integration to this integrations manifest. In addition, we cannot rely on event.dataset, we need to look for the logs-endpoint* index for this.

Mikaayenson · 2025-06-17T20:50:00Z

tests/test_all_rules.py

+        osquery_note_pattern = (
+            "> **Note**:\n> This investigation guide uses the [Osquery Markdown Plugin]"
+            "(https://www.elastic.co/guide/en/security/current/invest-guide-run-osquery.html) "
+            "introduced in Elastic Stack version 8.5.0. Older Elastic Stack versions will display "
+            "unrendered Markdown in this guide."
+        )
        invest_note_pattern = (
-            '> This investigation guide uses the [Investigate Markdown Plugin]'
-            '(https://www.elastic.co/guide/en/security/current/interactive-investigation-guides.html)'
-            ' introduced in Elastic Stack version 8.8.0. Older Elastic Stack versions will display '
-            'unrendered Markdown in this guide.')
+            "> This investigation guide uses the [Investigate Markdown Plugin]"
+            "(https://www.elastic.co/guide/en/security/current/interactive-investigation-guides.html)"
+            " introduced in Elastic Stack version 8.8.0. Older Elastic Stack versions will display "
+            "unrendered Markdown in this guide."
+        )


We should double check that when the transform occurs, its still formatted correctly.

Mikaayenson · 2025-06-17T20:58:16Z

detection_rules/beats.py

    print(f"Downloading beats {release_name}")
    response = requests.get(url)

    print(f"Downloaded {len(response.content) / 1024.0 / 1024.0:.2f} MB release.")

-    fs = {}
+    fs: dict[str, Any] = {}


should we type hint the parsed field below

Mikaayenson · 2025-06-17T20:59:08Z

detection_rules/beats.py

+# def get_schema_from_eql(tree: eql.ast.BaseNode, beats: list, version: str = None) -> dict:
+#     """Get a schema based on datasets and modules in an EQL AST."""
+#     datasets, modules = get_datasets_and_modules(tree)
+#     return get_schema_from_datasets(beats, modules, datasets, version=version)


Mikaayenson · 2025-06-17T21:01:13Z

detection_rules/cli_utils.py

+    suggested_path: Path = Path(DEFAULT_PREBUILT_RULES_DIRS[0]) / contents["name"]
+    path = Path(path or input(f"File path for rule [{suggested_path}]: ") or suggested_path).resolve()


Seems a bit odd to type hint as Path when we explicitly set as a Path object. We also dont type hint the next field path.

Mikaayenson · 2025-06-17T21:03:14Z

detection_rules/config.py

        """Format unit test names into expected format for direct calling."""
-        raw = [t.rsplit('.', maxsplit=2) for t in tests]
-        formatted = []
+        raw = [t.rsplit(".", maxsplit=2) for t in tests]


Suggested change

raw = [t.rsplit(".", maxsplit=2) for t in tests]

raw: list[list[str]] = [t.rsplit(".", maxsplit=2) for t in tests]

?

Mikaayenson · 2025-06-17T21:09:36Z

detection_rules/devtools.py

+        paths = [Path(c) for c in columns[1:]]
+        return cls(columns[0], *paths)


Is there a way to clean this up?

Mikaayenson · 2025-06-17T21:17:55Z

detection_rules/docs.py

-        worksheet.freeze_panes(1, 0)
-        worksheet.set_column(0, 0, 25)
-        worksheet.set_column(1, 1, 10)
+        worksheet = self.add_worksheet("Summary")  # type: ignore[reportUnknownMemberType]


Can we add this to the function # type: ignore[reportUnknownMemberType] instead of inline.

Mikaayenson · 2025-06-17T21:27:37Z

detection_rules/ecs.py

    """Get schema for KQL."""
-    indexes = indexes or ()
-    converted = flatten_multi_fields(get_schema(version, name='ecs_flat'))
+    indexes = indexes or []


Curious as to why this was a tuple

Mikaayenson · 2025-06-17T21:38:39Z

Any reason why the build didn't run? Waiting for status to be reported
Note, I think we need to run the lint tests locally and add to this PR (since the workflow won't run until the action is on main)
We'll also want to open a maintenance window and test the backporting logic.

traut changed the title ~~[WIP] Type hint fixes and adding code checks~~ [WIP] fix: type hint fixes and adding code checks Jun 11, 2025

Mikaayenson assigned traut Jun 16, 2025

traut force-pushed the style-fixes branch from 6f18d46 to 5cac576 Compare June 17, 2025 12:05

traut added python Internal python for the repository ci/cd maintenance Internal changes minor labels Jun 17, 2025

traut marked this pull request as ready for review June 17, 2025 16:32

traut requested review from Mikaayenson, eric-forte-elastic and terrancedejesus as code owners June 17, 2025 16:32

github-actions bot added the backport: auto label Jun 17, 2025

traut added 18 commits June 17, 2025 18:32

first pass

fb8d74c

Adding a dedicated code checking workflow

a08b46d

Type fixes

b9dabe4

linting config and python version bump

4418a2c

Type hints

999eaee

Drop incorrect config option

5748f78

More fixes

90e5b4e

Style fixes

becb193

CI adjustments

f4c7cc0

Pyproject fixes

33b53c3

CI & pyproject fixes

fa33ed9

Version bump

41f6433

Tests formatting

d2f947e

Resolve cirtular dependency

77edd83

Test fixes

1e0eecf

Make sure the tests are formatted correctly

3ff0b38

Check tweaks

abde0d5

Bumping python version in CI images

100734c

traut added 3 commits June 17, 2025 18:32

Pin marshmallow do 3.x because 4.x is not supported

e643fdb

License fix

80e5021

Convert path to str

4afd534

traut force-pushed the style-fixes branch from 4e69b4e to 4afd534 Compare June 17, 2025 16:32

botelastic bot added Hunting schema labels Jun 17, 2025

traut changed the title ~~[WIP] fix: type hint fixes and adding code checks~~ fix: type hinting fixes and additional code checks Jun 17, 2025

Making myself a codeowner

968afaf

Mikaayenson reviewed Jun 17, 2025

View reviewed changes

		suggested_path: Path = Path(DEFAULT_PREBUILT_RULES_DIRS[0]) / contents["name"]
		path = Path(path or input(f"File path for rule [{suggested_path}]: ") or suggested_path).resolve()

	raw = [t.rsplit(".", maxsplit=2) for t in tests]
	raw: list[list[str]] = [t.rsplit(".", maxsplit=2) for t in tests]

		paths = [Path(c) for c in columns[1:]]
		return cls(columns[0], *paths)

fix: type hinting fixes and additional code checks #4790

Are you sure you want to change the base?

fix: type hinting fixes and additional code checks #4790

Conversation

traut commented Jun 11, 2025

Pull Request

Summary - What I changed

How To Test

Checklist

Contributor checklist

Uh oh!

github-actions bot commented Jun 17, 2025

Enhancement - Guidelines

Documentation and Context

Code Standards and Practices

Testing

Additional Schema Related Checks

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mikaayenson Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mikaayenson Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mikaayenson commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Mikaayenson Jun 17, 2025 •

edited

Loading

Mikaayenson Jun 17, 2025 •

edited

Loading

Mikaayenson commented Jun 17, 2025 •

edited

Loading