[high] Write finalized, postprocessed queries with --output-dir - #105
Merged
thomaspatzke merged 3 commits intoSep 27, 2026
Merged
Conversation
write_separate_files collected queries through the per-condition callback of backend.convert(), which runs before finalize_query (output format and pipeline query postprocessing) and also for rules that are not output. Read the finalized conversion result of each output rule after the conversion instead. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
thomaspatzke
requested review from
thomaspatzke
and
a lite review from Copilot
September 27, 2026 11:13
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
write_separate_files() still swallows backend.convert() exceptions and can exit 0 after a conversion failure, which can yield incomplete output without failing the command.
Review effort: Lite
Findings: 1
What changed in this PR
This PR fixes --output-dir so it writes the same finalized, postprocessed per-rule queries that sigma convert would normally output, including selected output format rendering and pipeline query postprocessing, and ensuring non-output correlation base rules are not written.
Changes:
- Switch
--output-dirto write each rule’s finalizedrule.get_conversion_result()(only forrule_collection.get_output_rules()) instead of using the backend’s per-condition callback. - Update/extend CLI tests to assert output-format finalization, pipeline postprocessing, and skipping of non-output correlation base rules.
- Adjust the correlation
--output-dirtest description/expectations to match the “output rules only” behavior.
| File | Description |
|---|---|
sigma/cli/convert.py |
Stop using the backend callback for --output-dir; write finalized conversion results from output rules instead. |
tests/test_convert.py |
Add regression tests ensuring --output-dir output matches finalized/processed single-output behavior and skips non-output base rules. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Member
|
@copilot resolve the merge conflicts in this pull request |
elhoim
added a commit
to elhoim/sigma-cli
that referenced
this pull request
Sep 28, 2026
Resolve conflicts with upstream main, keeping both the upstream changes and this PR's change. Upstream (SigmaHQ#105) now collects the finalized result of every output rule after backend.convert() instead of using the per-condition callback. Those results are now grouped per rule object, as this PR does, and passed to this PR's collision-aware writer; rules without results are skipped. test_convert_output_dir_skips_non_output_correlation_base_rules expected 'Wrote 3 file(s)' for three correlation rules from one source file. All three render to the same file name, so upstream overwrote one file three times and kept only the last query. With this PR they are joined into that one file. The test now expects 'Wrote 1 file(s)' and checks that the file content equals the -o output, which still shows that base rules are not written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thomaspatzke
pushed a commit
that referenced
this pull request
Oct 4, 2026
load_and_check_rules() only looked at the errors of the rules in SigmaCollection.rules. Errors that are not attached to a rule, such as an unknown collection 'action:' or an invalid filter (filters are kept in SigmaCollection.filters), were never reported, so 'sigma check' passed inputs that 'sigma convert' rejects. These errors are now reported, counted as rule errors (so --fail-on-error applies) and added to the JUnit report. Rule errors are also contained in SigmaCollection.errors; they are skipped there so they are not counted twice. Re-applies #99 (cherry picked from commit 0d0231f), which was merged but then undone on main: the merge of #105 (8effb4c) brought in cdd5436, a revert of a conflict-resolution merge (86d3041) that had carried #98 and #99 into the #105 branch. #99 did not conflict with #105; it was only caught by that revert. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
thomaspatzke
pushed a commit
that referenced
this pull request
Oct 4, 2026
Re-applies #98, which was merged but then undone on main: the merge of #105 (8effb4c) brought in cdd5436, a revert of a conflict-resolution merge (86d3041) that had carried #98 and #99 into the #105 branch. Adapted to #105's write_separate_files(): rules are converted one by one without a callback, so a failing rule is reported and counted instead of aborting the conversion of every later rule behind a warning. Each successful conversion stores its finalized result on the rule, which #105 then collects via get_output_rules()/get_conversion_result(). The command exits 1 when any rule failed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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
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.

BLUF
--output-dirwrites the queries before they are finalized. The chosen--format, every query postprocessing item of the processing pipelines (embed,template,nest, ...) and the backend's per-query finalizer are skipped. Base rules of correlations that are not output (generate: false) are written too.-oand-od. For example, a pipeline that embedsindex=prod (...)scopes the query with-o, but the file written with-odholds the bare query that runs over all indexes. Withtests/files/sigma_correlation_rules.yml,-ogives 3 queries and-odwrites 6.rule.get_conversion_result()) of each rule returned byrule_collection.get_output_rules().mainand pass with the fix. The full suite passes (121 passed, 1 skipped).Priority: high
Details
Root cause:
sigma/cli/convert.py:151-195(regression from d0dfcdf, "Support correlation rules with --output-dir using callback mechanism").write_separate_files()only keeps what thecallbackofbackend.convert()receives. In pySigma,Backend.convert_rule()calls that callback right afterfinish_query()(sigma/conversion/base.py:274-275). Everything below happens after it:finalize_query()at lines 281-290, which runsfinalize_query_<format>and the pipeline'spostprocess_query()if rule._outputcheck at line 297convert_correlation_rule()has the same order (callback at 744-745,finalize_queryat 750-758). pySigma documents the callback as "called for each condition conversion", so it is meant to see intermediate results. This is how the CLI uses the API, not a pySigma bug.After
backend.convert()returns, pySigma has stored the finalized queries on each rule (rule.set_conversion_result(finalized_queries)). The fix reads those, and only for rules inget_output_rules(), which is the same setconvert()returns queries for. Rules that were not converted (conversion failed) have no result and are skipped.get_output_rules(),get_conversion_result()andSigmaConversionErrorall exist in pySigma 1.3.0, the lower bound of the dependency pin.Behavior change: base rules referenced by a correlation rule are no longer written unless they have
generate: true. This matches-o. The comment intest_convert_output_dir_with_correlation_rulessaid files were expected "for base rules and correlation rules". I updated it.Out of scope: the collection-level
finalize_output_<format>and the pipelinefinalizers(pipeline.finalize()) work on the whole rule set, not on a single query. They are still not applied per file. For example,-f savedsearches -odnow writes the savedsearches stanza of each rule, but not the[default]header that the collection-level finalizer adds.Overlap with open PRs:
backend.convert(...)call with a per-ruleconvert_rule()loop that still uses the callback.Both will conflict textually with this change, but they compose. With #98, the loop can read
rule.get_conversion_result()after eachconvert_rule()instead of using the callback. Whichever PR lands second needs a mechanical rebase. Theexcept Exceptionthat swallows conversion errors is left as is here, because #98 handles it.Testing
tests/test_convert.py::test_convert_output_dir_applies_output_format:-f test -odwrites[ ... ], the same as-f testto stdout.tests/test_convert.py::test_convert_output_dir_applies_pipeline_postprocessing: a pipeline with anembedpostprocessing item wraps the written query.tests/test_convert.py::test_convert_output_dir_skips_non_output_correlation_base_rules: "Wrote 3 file(s)" instead of 6.mainand pass with the fix.pytest --cov=sigma --cov-report term --cov-report xml:cov.xml -vvon Python 3.12: 121 passed, 1 skipped.🤖 Generated with Claude Code