Repository navigation
Complete detection-quality edge-case acceptance coverage - #260
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92bce974b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ev.get(req) is None: | ||
| missing_fields_in_case.add(req) |
There was a problem hiding this comment.
When a negative case contains multiple primary events—one with a null required field and another complete event that matches—the case-wide null collection classifies the entire result as missing_field_error even though actual_match is true. The suite then excludes it from false positives and treats expected-negative schema errors as passing, allowing a noisy rule to clear the gate; preserve the false-positive outcome whenever an evaluable event matched, or track schema and match outcomes independently.
AGENTS.md reference: plugins/AGENTS.md:L9-L13
Useful? React with 👍 / 👎.
| if not any( | ||
| secondary.get("source") in secondary_sources | ||
| and all(event.get(key) is not None and event.get(key) == secondary.get(key) for key in join_keys) | ||
| for secondary in case.events |
There was a problem hiding this comment.
When a rule declares multiple secondary sources and inner joins, any(...) admits a primary event as soon as one secondary source matches the keys. A fixture missing another required secondary row can therefore be reported as a match even though the actual sequence of inner joins would reject it; group matching rows by source and require a match for each declared joined source.
AGENTS.md reference: plugins/AGENTS.md:L9-L13
Useful? React with 👍 / 👎.
| for case in cases: | ||
| for event in case.events: | ||
| present_keys = set(event.keys()) | ||
| if event.get("source") not in (None, rule.source_data["primary_source"]): |
There was a problem hiding this comment.
A rule whose source_data omits primary_source is still accepted by Rule.from_dict and the CLI validate command, but any nonempty fixture now reaches this direct lookup and raises an uncaught KeyError because the CLI only handles QualityError. Validate the nested source contract during rule loading, or convert this failure into a controlled validation error, so a rule reported as valid cannot crash evaluate or test-suite.
Useful? React with 👍 / 👎.
| def test_time_window_is_inclusive_and_excludes_future_events(rule: Rule, event_time: str, expected: bool) -> None: | ||
| fixture = case("window", (sign_in(TimeGenerated=event_time),), expected, evaluation_time="2026-10-01T12:00:00Z") | ||
| assert evaluate_fixture_case(fixture, rule).actual_match is expected |
There was a problem hiding this comment.
This commit introduces behavioral requirements for joins, null telemetry, time boundaries, deduplication, and suppression only as package-level pytest tests; no corresponding specs/ feature or executable Gherkin scenario is added. Add matching pytest-bdd scenarios and step definitions so these acceptance criteria participate in the repository's required specification gate rather than existing only as implementation tests.
AGENTS.md reference: AGENTS.md:L162-L163
Useful? React with 👍 / 👎.
| tenant_id = event.get("TenantId") | ||
| if not isinstance(tenant_id, str) or not tenant_id or tenant_id != alert.get("tenant_id"): | ||
| continue | ||
| if earliest <= alert_time <= event_time: |
There was a problem hiding this comment.
The new prior-alert path makes tenant- and identity-scoped decisions that can suppress otherwise matching detections, but the detailed threat model remains unchanged despite this new security-relevant control and its missing-tenant assumptions. Update its data-flow evidence, tenant-confusion abuse cases, controls, validation, and unresolved assumptions, or record the required specific no-impact rationale if the model truly remains unchanged.
AGENTS.md reference: AGENTS.md:L187-L191
Useful? React with 👍 / 👎.
| "case_id": "CASE-SUPPRESSED-ALERT", | ||
| "description": "A prior alert for the same user and tenant suppresses a repeated match", | ||
| "label": "negative", | ||
| "expected_match": false, |
There was a problem hiding this comment.
This fixture contains an event that satisfies the malicious sign-in predicate and is omitted only because a prior alert suppresses a duplicate notification, but labeling it negative makes the evaluator count it as a true negative that the report describes as benign activity. The bundled TN total therefore mixes benign telemetry with detected-but-suppressed attacks; introduce a separate suppression outcome outside the confusion matrix, or otherwise keep detection truth separate from notification emission.
AGENTS.md reference: plugins/AGENTS.md:L9-L13
Useful? React with 👍 / 👎.
| "identity_scope": { | ||
| "type": "string" | ||
| "type": "string", | ||
| "enum": ["global", "user", "host"] |
There was a problem hiding this comment.
The previous v1 schema accepted any string for identity_scope, while this enum and the corresponding parser check now reject existing v1 documents that use another scope—even when suppression is disabled—without changing the cops.detection-rule/v1 discriminator. Bump the rule contract and provide a migration path, or preserve v1 parsing compatibility and apply the narrower validation only under a new schema version.
AGENTS.md reference: AGENTS.md:L132-L134
Useful? React with 👍 / 👎.
1 participant
Summary
Complete the remaining acceptance coverage in #194 for the offline detection quality workbench. Add scoped reference semantics and fixtures for cross-tenant joins, null required fields, timestamp boundaries, duplicate events, and prior-alert suppression. The bundled Sentinel query remains unchanged; the bundled Splunk rule declares no suppression because its source has no tenant identifier.
Closes #194.
Acceptance criteria
make checkpassed; affected user and operator docs were updated.Evidence
venv/bin/python -m pytest -q tests/test_workbench.py tests/test_edge_cases.pyin the package.make checkpassed in this worktree. The local push gate also passed at commit92bce974b0b6618e9ddc7ecdd39486073fe5c98b, including Ruff 0.16.3, Semgrep 1.179.0, and dependency audit.Impact review
cops.detection-quality-report/v2; readers of v1 need to handle schema-error counts and nullable precision/recall.Critique disposition
Independent automated review identified that the original Splunk suppression metadata could not enforce tenant scope because the source has no
TenantId. The rule now declares no suppression, and a regression fixture verifies that a same-host prior alert does not suppress it. The final diff was self-reviewed; automated review does not satisfy the required human review.Risks and residual uncertainty
The reference evaluator models fixture outcomes and does not execute KQL or SPL or query alert history. Target-SIEM execution and production data behavior remain unverified. Human exact-head review is required before merge.