Skip to content

chore: add json/ndjson/jsonl to regression test - #6203

Open
pH-T wants to merge 1 commit into
SigmaHQ:masterfrom
pH-T:master
Open

chore: add json/ndjson/jsonl to regression test#6203
pH-T wants to merge 1 commit into
SigmaHQ:masterfrom
pH-T:master

Conversation

@pH-T

@pH-T pH-T commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary of the Pull Request

Adds JSON/NDJSON/JSONL support to the current SigmaHQ regression testing implementation (right now only EVTX files are supported).

TL;DR

Changelog

chore: add json/ndjson/jsonl to regression test

Example Log Event

Fixed Issues

SigmaHQ Rule Creation Conventions

  • If your PR adds new rules, please consider following and applying these conventions

@github-actions github-actions Bot added Review Needed The PR requires review Maintenance Related to additions and update of the repository features labels Aug 6, 2026
@pH-T
pH-T force-pushed the master branch 2 times, most recently from db0cda6 to d72b869 Compare August 6, 2026 11:21
@phantinuss
phantinuss requested a lite review from Copilot August 6, 2026 11:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds JSON/NDJSON/JSONL regression-test support alongside existing EVTX regression testing by extending the Python runner, adding a Sigma conversion pipeline, updating sample info.yml files, and wiring dependencies/tools into CI.

Changes:

  • Extend tests/regression_tests_runner.py to compile rules to golang_expr via sigma convert and execute json_checker for json/ndjson/jsonl samples (including pipeline/filter support).
  • Add regression-test documentation and a new conversion pipeline file for field mapping.
  • Update CI workflow and a few rule info.yml files to exercise JSON regression tests.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/regression_tests_runner.py Adds JSON/NDJSON/JSONL execution path via sigma-cli conversion + json_checker; adds CLI flag and passes pipeline/filter paths through.
regression_data/rules/windows/registry/registry_set/registry_set_add_load_service_in_safe_mode/info.yml Adds a JSON-based regression test entry using the new pipeline.
regression_data/rules/windows/process_creation/proc_creation_win_autologger_session_registry_modification/info.yml Adds a JSON-based regression test entry using the new pipeline.
regression_data/rules/windows/process_creation/proc_creation_win_amsi_registry_tampering/info.yml Adds a JSON-based regression test entry using the new pipeline.
regression_data/README.md Introduces documentation for regression data layout and supported test types (EVTX + JSON variants).
regression_data/pipelines/process_creation_fieldmapping.yml Adds a Sigma pipeline intended to map JSON fields into Sigma’s expected field names.
.github/workflows/regression-tests.yml Installs sigma-cli backend deps, downloads json_checker, and runs the runner with --json-checker.
Suppressed comments (3)

regression_data/pipelines/process_creation_fieldmapping.yml:20

  • Same issue here: rule_conditions is nested under mapping, which will likely prevent the transformation from being correctly scoped to the intended logsource.
      mapping:
          TargetObject: Event.EventData.TargetObject
          Details: Event.EventData.Details
          Image: Event.EventData.Image
          rule_conditions:

tests/regression_tests_runner.py:433

  • CLI help text has a typo ('ndjsonjson') and doesn’t mention jsonl, even though run_test supports it. This can confuse users about when --json-checker is required.
    parser.add_argument(
        "--json-checker",
        help="Path to json_checker binary (required for 'ndjsonjson' tests)",
    )

tests/regression_tests_runner.py:482

  • Comment is outdated/incomplete: json_checker is needed for jsonl tests as well (not just ndjson/json). Keeping this accurate matters because it’s part of the runner’s user-facing guidance.
        # json_checker is optional; only needed for 'ndjson/json' tests
        if args.json_checker and not os.path.exists(args.json_checker):
            print(f"Error: json_checker not found at {args.json_checker}")

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread regression_data/pipelines/process_creation_fieldmapping.yml Outdated
Comment thread tests/regression_tests_runner.py Outdated
Comment thread regression_data/README.md
@pH-T
pH-T force-pushed the master branch 2 times, most recently from 3600f2f to 3d01bbc Compare August 6, 2026 14:20
Comment on lines +372 to +376
if match_count > expected_count:
print(
f" Warning: {rule_id}: Got {match_count} matches but only {expected_count} expected - consider updating match_count in info.yml"
)
return True, all_output

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be an error as well. As to not bloat the logs. We should only consider the "malicious" dataset

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% agree but its the same for evtx atm (see line 288). this would have been a future proposal from my side.
so i fix it for both? (--> it should be an error)

Comment on lines +85 to +87
filters = [
get_absolute_path(base_dir, f) for f in test.get("filters", [])
]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats the though behind including filters in this. Can you share an example you think we might apply filters?

Imo since this is only true positive testing (AKA intentional malicious matches). This should not be here.

Thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the idea is that ppl can use this to "regression test" their env specific FP tunings in their local repo/ci.

i dont think this will be used in the SigmaHQ repo itself.

Comment thread regression_data/README.md
Comment on lines +49 to +50
`json` means a single JSON object, while `ndjson`/`jsonl` mean newline-delimited
JSON objects (one object per line).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need some guidelines defined around json files that are accepted in this repo.

While its true that any json correct file can be valid, we cannot allow that to happen as we could have heterogeneity that is unmaintainable.

Imo we should define a set of accepted types (that are for example generated by different tooling). Exports from SIEM X, EDR Y, or tool Z and then we can define a schema for those as well later down the line to have some sort of control.

And I think this is a good idea not only for HQ but in general for anyone trying to replicate this on their own

Thoughts?

cc @phantinuss @pH-T

@phantinuss phantinuss Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the JSON logs I mainly think about all the cloud logs which are natively JSON. I don't think the workload of creating/finding the (correct) schemas is sth we can handle. While I understand the approach and its benefits I am not sure we have the resources to maintain the schemas. We would also lose flexibility when we have a new source we have to add a schema before the first addition is possible.

What is the fear of drift? We can still just decline json logs that e.g. clearly are synthetic.

Comment thread regression_data/pipelines/process_creation_fieldmapping.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Maintenance Related to additions and update of the repository features Review Needed The PR requires review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants