[gapilongo]

Closes #343

Closure cost, plainly

With analysts deciding every escalation correctly (oracle, corpus_v1):

Mode	Benign closes, before	Benign closes, after
propose	82 of 312	11 of 312
accept	85 of 312	31 of 312

• In propose mode, verdict-cache closes fall from 80 to 7.
• Malicious closes are 0 of 208 in both modes, down from 4.

The safety costs most of what the cache and suggestions closed.

Summary

Safety fix, tier T3. The analyst verdict cache and the suppression rules suggested from false positives matched alerts by fingerprint only: the rule plus hosts, users, IPs, hashes and domains. With analysts deciding every escalation correctly, corpus_v1 still closed 4 attacks next to benign activity on the same host:

• cv1-0113: a ransomware PSEXESVC service, after an Edge update service.
• cv1-0309 and cv1-0440: dropped services.
• cv1-0494: a web shell's ps aux running as uid 33, after monitoring's ps running as root.

Verdict key (domain/verdict_key.py, recipe v4). The key is the fingerprint plus what ran:

• process, command line, service and user/uid;
• parent process, including ECS process.parent.* and Defender InitiatingProcess*;
• the objects an attacker creates: scheduled task name and content, registry key, target file, and the WMI name, filter and consumer in Sysmon 19-21 events.

Request context is in the key but never anchors one. These fields alone never produce a key, because whoever sends the request sets them:

• URLs and request paths/queries;
• user agents;
• O365 operations and their Parameters;
• DNS and WMI queries and destinations;
• registry value details and file paths;
• Sysmon 10 GrantedAccess and CallTrace.

Each field keeps its own name, so a subject and a target user cannot swap unnoticed.

• Compared in full. Only whitespace, doubled backslashes and Windows-path case are normalised. Nothing is masked or cut.
• No key (so no cache close, no suggestion and no inherited benign group decision) when:
  • the event names no process, command, service or created object (context alone is not enough);
  • the alert is in a FIX-1 never-close class. Unmapped alerts are the exception: analysts decided this exact behaviour, and unmapped detections are mostly routine noise;
  • the alert was normalised by a model;
  • the event was truncated, or a value is over 16 KiB.

Verdict cache v3 (core/verdict_cache.py)

• A close needs two different analysts (decided_by) within 7 days, at the alert's severity or higher. Critical alerts never close from the cache.
• One analyst's decisions count once: a group decision, a bulk close, or deciding twice.
• A decision without a known analyst counts as one unknown analyst.
• A threat writes a 7-day tombstone on both the key and the fingerprint. Old key versions are never read.

Suppressions

• Suggestions match verdict_key, for both single false positives and bulk proposals. A bulk proposal for an alert without a key returns 422 no_verdict_key.
• A confirmed threat disables every enabled rule that would close the alert, audited as suppression.revoked. A confirmed threat is any of:
  • an analyst true_positive;
  • a blind-audit malicious label;
  • a planned response;
  • a malicious close;
  • the oracle's true_positive.
• A failed revocation in the API is logged at error level and counted in lgsotf_revocation_failures_total.
• Migration 0016 disables enabled fingerprint-only suggestions. Each one is audited in its tenant's hash chain, by migration-0016 with reason fingerprint_suggestion. The gate also ignores such rules.
• Model-normalised alerts skip suppression rules at the gate.

Grouping: an attached alert inherits a benign group decision only when it has the same verdict key. Ingestion dedup never used the fingerprint.

Oracle results (corpus_v1, fake model, decision policy v2 at tier off, two oracle analysts)

"Before" is today's code with the cache and suggestions keyed by fingerprint, and one decision enough.

Run	Benign closes (Wilson 95% CI)	Malicious closes
propose, before	82 of 312, 26.3% (21.7–31.4%); cache 80, model 2	4 of 208 (cv1-0113, -0309, -0440, -0494)
propose, after	11 of 312, 3.5% (2.0–6.2%); cache 7, model 4	0 of 208 (0.0–1.8%)
accept, before (227 rules enabled)	85 of 312, 27.2% (22.6–32.4%); suppression 83, model 2	4 of 208
accept, after (104 rules enabled)	31 of 312, 9.9% (7.1–13.8%); suppression 27, model 4	0 of 208 (0.0–1.8%)

What it costs. Most of the lost closes come from alerts that no longer get a key:

• new services, scheduled tasks, account changes and brute force are never-close classes;
• sign-in and audit alerts name users only.

Requiring two analysts and comparing command lines unmasked cost the rest. With one analyst enough, the unit replay on the new key closes about 20 benign alerts and still 0 attacks.

Tests

• Acceptance: tests/integration/eval/test_oracle_corpus_v1.py::test_the_oracle_closes_no_attack_on_corpus_v1[propose|accept] passes. It calls run_evaluation with the oracle on, as lg-sotf eval --oracle does.
• Re-review N1: regressions for a Splunk SQL injection behind a proxy with a benign search's user agent, an O365 forward rule next to a sorting rule, and lsass access 0x1000 vs 0x1FFFFF (test_verdict_key_review.py, test_verdict_key.py::test_context_alone_never_makes_a_key).
• Re-review N2: tests/unit/core/test_gate.py covers fingerprint_suggestion() and the gate skip, including a fingerprint-only rule an admin enabled again.
• Re-review N5: the bulk proposal dialog explains 422 no_verdict_key (vitest and tsc -b pass).
• Reviewer's failing tests: adopted in tests/unit/domain/test_verdict_key_review.py and passing.
  • The scheduled-task test now also asserts that the T1053-mapped variant gets no key.
  • The helper names two analysts, because a cache close needs them.
• Probe collisions: the reviewer's probes are in tests/unit/domain/test_verdict_key.py::test_attacker_writable_differences_never_share_a_key.
• Revocation through real paths:
  • tests/integration/api/test_revocation.py covers the API true positive, the blind-audit label, and the failure counter.
  • tests/integration/engine/test_engine.py::test_plan_response_and_the_malicious_close_revoke_the_benign_learning covers plan_response and the malicious close.
• Migration: tests/integration/db/test_migrations.py::test_0016_... checks the disabled rules and that the audit chain still verifies.
• Checks: ruff, global strict mypy and lint-imports (14 kept) are green; unit, deploy and invariants suites pass.

Threat note (T3)

• Weakness: the cache and suggested rules used a key that did not identify the activity, so an attack next to benign noise of the same rule closed unseen.
• Fix: the key binds what ran, compared in full. No key exists when there is nothing to bind, or for never-close classes. A cache close needs two different analysts. A threat tombstones the key and fingerprint and disables matching rules; failures are counted.
• Old suggestions: fingerprint-only suggestions are disabled by migration 0016 and ignored at the gate.
• Residual (L2): revocation evaluates regex rules within the normal work budget, so a rule whose regex runs out of budget is not revoked.
• Residual: detection-report proposals stay rule-wide by design. Admin-written fingerprint rules with other predicates are left alone.

Merge notes

• Migration number: this PR adds migration 0016. Open PR AI-E1.5c+d: shadow SELECT-only DB role, separate LLM slots, gate and F6 #323 also adds a 0016 and will renumber when it merges after this one.
• Unmerged work carried: this branch carries the unmerged AI-E1.9 ([AI-E1.9] Auto-close calibration: benign alerts should close without weakening the zero-miss bound #316) and fatigue-1 commits, merged for the analyst oracle.
• Main merged: origin/main with FIX-1 (FIX-1: decision policy v2 (evidence gate replaces the rules-score gate) + per-tenant autonomy tier #341) is merged. Conflicting hunks keep FIX-1's evidence gate, tier, never-close classes and correlation-as-evidence. The corpus_v1 cassettes were re-recorded with the fake model.
• AI-E1.9 code FIX-1 superseded is removed:
  • the policy simulator (only decision_inputs stays);
  • the event-time correlation clock setting, so reports no longer record correlation_clock;
  • the knob tests;
  • the obsolete changelog and system-card text.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JmdYVXkgpNHoygVJCrUeZc

[claude]

Review of #344 (verdict cache and suppressions keyed by behaviour)

This is a real fix for a real class of bug, and the design is sound. The verdict key binds behaviour, a close needs k=2 decisions, and a threat leaves tombstones on both the key and the fingerprint. Failure handling is mostly fail-closed (truncated events, unreadable tombstone times, critical alerts). The oracle numbers (4 malicious closes → 0) and the replay regression tests are the right evidence for a T3 change.

I read domain/verdict_key.py, core/verdict_cache.py and core/benign_learning.py in full. The diff is over 300 files, so GitHub's diff API refuses it, and I did not review everything. I did not run the tests.

Security and correctness

1. Long commands collide on their prefix (medium). normalize_value caps every value at MAX_VALUE_CHARS = 512 after masking, and cuts the input to 4096 chars before that. A malicious command that starts with the same 512 chars as an analyst-approved one gets the same key (a long powershell -c ... or bash -c line with the payload appended). That is the collision class this PR closes, moved elsewhere. Options: hash the full normalised value and keep a short prefix only for display, or return None (no key) when a value exceeds the cap, in line with the fail-closed policy used elsewhere.

2. Masking can erase attacker-relevant differences (low to medium). _HEX (16+ hex chars) and _DIGITS (6+ digits) apply to command lines, so inline hex payloads, hash arguments and numeric ids collapse to <hex> or <n>. _mask_temp collapses any temp-dir name containing a digit or starting with tmp/~, so \Temp\a1.exe equals \Temp\update2.exe. Consider masking only where volatility is expected, and add negative tests with attacker-chosen tokens for each mask.

3. Alerts with no behaviour fields get a fingerprint-only key (low). If no named field is found, behaviour() returns {} and the key is effectively the old fingerprint key. k=2 and the tombstones still apply, but I would rather such alerts not use the cache. If intentional, document it and add a test.

4. "Distinct analyst decisions" means distinct escalations, not distinct analysts (low). One analyst closing two escalations meets k=2. If independence is the goal ("two independent analyst decisions" in the Threat note), key on the analyst too, or reword the claim.

5. put_analyst_benign is not atomic (low). hsetnx, hgetall, hdel and expire run outside a pipeline, so a concurrent invalidate can interleave. The tombstone should still win because closing_decisions checks threats first. A short comment would help. The expire on each write refreshes the tombstone's Redis TTL, which only lengthens the block (the safe direction).

6. Legacy rules still match by fingerprint (acknowledged residual). Could a migration or the admin UI flag rules without a verdict_key as legacy so they get replaced? Disabling them only on the next threat leaves a window.

7. Revocation is skipped when regex evaluation runs out of budget (acknowledged residual). Consider auditing a skip event so it is visible rather than silent.

Scope and process

• The PR carries unmerged work from other branches (AI-E1.9 [AI-E1.9] Auto-close calibration: benign alerts should close without weakening the zero-miss bound #316, fatigue-1, scorecards, AITADS fixtures, re-recorded cassettes) and touches more than 300 files. The description also says the event-time correlation clock is inactive after the merge. That is very hard to review and to revert. Please split it: land [AI-E1.9] Auto-close calibration: benign alerts should close without weakening the zero-miss bound #316 and fatigue-1 separately, then rebase so this PR holds only the verdict-key, cache, suppression and docs changes. A safety fix should be easy to bisect and backport.
• The changelog fragments AI-E1.9.md and fatigue-1-measurement.md belong with their own PRs.

Tests

Coverage is strong: the corpus replay, the fingerprint-collision guard and the oracle acceptance tests. I would add boundary tests for the 512-char cap and each mask (items 1 and 2), a test that an alert with no behaviour fields gets no cache close (item 3), and a concurrency test for put_analyst_benign racing invalidate.

Style

The code is typed, short and well documented. The Dict/List imports from typing could be the built-in dict/list, as clear_tenant already does with list[Any].

[claude]

Review of #344: verdict key and verdict cache v3

Scope. The diff is too large for the GitHub API (353 files, about 5.6k lines added). I read domain/verdict_key.py, core/verdict_cache.py and migration 0016 in full. I did not run the test suite, and I did not review the rest of the diff (API, gate, engine, eval, cassettes). Treat this as a partial review.

The design is sound: it fails closed in the right places, and the description and threat note are clear. Comments below, in rough order of importance.

Issues

1. normalize_value contradicts the "nothing masked" claim (domain/verdict_key.py).

   • re.sub(r"\\{2,}", r"\\", ...) runs on every field, including Linux command lines and OBJECT fields. printf '\\n' and printf '\n' are different commands in the shell but get the same key.
   • Doubling is only a JSON-escaping artefact for Windows paths. Restrict the step to values matching _WINDOWS_PATH.
   • Whitespace collapsing has the same issue inside quoted arguments ("a b" vs "a b"). Lower risk, but worth narrowing or documenting.

2. UNC paths are never lowercased. The backslash collapse runs first, so \\server\share becomes \server\share and _WINDOWS_PATH no longer matches the UNC prefix. This is a false miss, so it is safe but inconsistent. Test for the path before collapsing.

3. Cache writes are not atomic (put_analyst_benign). It runs hset, hgetall, hdel, expire separately. A crash between hset and expire leaves a key with no TTL. Put the write and the expiry in one pipeline.

4. invalidate uses a transactional pipeline over two keys. On Redis Cluster the verdict key and the fingerprint key probably hash to different slots, so MULTI fails with CROSSSLOT and revocation would fail. Add a tenant hash tag if cluster is supported; otherwise add a comment saying it is not.

5. Analyst identity is compared raw. The hash field is u:{decided_by} with no normalisation. If decided_by can arrive in different forms (email case, id vs email), one human counts as two analysts, and the cache's safety rests on MIN_ANALYSTS = 2. Please confirm it is a canonical id at every call site.

6. closing_decisions sorts on the ISO string. That is only correct while every timestamp has the same offset and format, and _time accepts any offset. Sort on the parsed datetime.

7. Regex-budget revocation failures are silent (the residual noted in the PR). Add a metric or audit entry, like the one for failed API revocations, so a pathological rule that survives a confirmed threat is visible.

8. Migration 0016 looks correct; the audit insert follows the repo's chain logic under the same advisory lock. Two notes:

   • It holds the audit-chain lock per tenant for the whole transaction. Note the expected volume in the docstring.
   • source_feedback_id IS NOT NULL also disables rules an admin later edited to add other predicates. I assume that is intended; please confirm.

Positives

• Fail-closed handling throughout: unreadable or naive threat times block the key, and truncated or oversized documents give no key.
• Each field keeps its own name, so subject and target users cannot swap.
• The tombstone covers both the key and the fingerprint, and old key versions are never read.
• 0 of 208 malicious closes in the oracle run, reported with a CI (it bounds the rate at 1.8%, not zero).

Test coverage

Coverage described in the PR is good. Gaps I would add:

• Key collisions for backslash and whitespace differences in Linux commands (issue 1).
• Concurrency between put_analyst_benign and invalidate.
• decided_by variants of one analyst counting once.
• A surfaced regex-budget revocation failure.

Process

The PR mixes a safety fix with merged AI-E1.9 and fatigue-1 work and removals (policy simulator, correlation clock setting). That is hard to review and revert. Consider splitting it, or at least listing the removed surfaces in the changelog.

[claude]

Review of PR 344: verdict key and verdict cache v3

The diff is over 300 files, so GitHub wouldn't serve it whole. I read the core of the change in the merge checkout: domain/verdict_key.py, core/verdict_cache.py, migration 0016, and fingerprint_suggestion in domain/suppression.py. I did not read the eval harness, fixtures, cassettes or front-end change, and I did not run the tests. There is no root CLAUDE.md, so I judged style by the surrounding code.

Overall: the design is sound. Every failure mode I looked at fails closed: unreadable threat time, truncated document, oversized value, LLM-normalised alert. The docstrings are clear, and the before/after numbers in the description are helpful.

Things worth a look

1. Unknown analyst plus one real analyst reaches MIN_ANALYSTS = 2 (verdict_cache.py). A decision with no decided_by is stored under u:? and counts as a separate analyst. If it came from the same person (a run paused before decided_by was recorded), one person satisfies the two-analyst rule. Consider not counting u:? toward the minimum, or add a test showing it is deliberate.

2. put_analyst_benign writes are not atomic. hset, hgetall, hdel and expire are separate calls. A crash between hset and expire leaves a key with no TTL, and the MAX_DECISIONS cap is best effort under concurrency. invalidate already uses a transaction pipeline. Risk is low because decision age is also checked, but keys can leak.

3. fingerprint_suggestion ignores any rule with fingerprint and source_feedback_id, even if it also has a verdict_key predicate. An admin who edits a feedback-derived rule to add verdict_key will see it silently never apply. Consider requiring that verdict_key is absent from the match keys, or document this in the user guide.

4. Migration 0016. FOR r IN UPDATE ... RETURNING is valid PL/pgSQL and the audit lock matches AuditRepo.append. app.bypass_rls is restored with COALESCE(previous, ''), so it ends as an empty string rather than unset; please confirm the RLS policies treat those the same. A migration test on a populated audit chain would help: the hash chain should still verify, and rules with other predicates should be untouched.

5. verdict_key.py details

   • _walk accepts only str and int scalars, so float values are silently dropped from the key, which makes it coarser. A comment or str() fallback would settle it.
   • _norm strips _, - and ., so process.name, process_name and processname merge. Probably intended; a test would document it.
   • Generic filename and folderpath are in the PROCESS role, so file events can anchor a key on a file name, not on something that ran. Exact match still keeps the close safe.
   • behaviour() and verdict_key() both run _fields and repeat the META filter; a shared helper would remove the duplication.

6. Observability: a metric separating 'no key: unanchored' from 'no key: caps or truncation' would help operators see why the cache stopped closing alerts. metrics.py is touched, so this may already exist.

Tests

test_verdict_key.py, test_verdict_key_review.py, test_verdict_cache.py and test_verdict_cache_replay.py exist, and the corpus replay is the right end-to-end check. I couldn't inspect the cases. If not already covered, please add: unknown plus known analyst; the same analyst on two alerts counts once; a fingerprint tombstone blocks a different key with the same fingerprint; the 16 KiB and 32/33-value boundaries; a feedback-derived rule with a verdict_key predicate; the 0016 migration on a populated chain.

Security

I found no injection or privilege problems in what I read. Redis keys are built from a hex digest and tenant id, and clear_tenant escapes [ and ]. Please confirm tenant ids cannot contain * or ?, or escape those too.

Housekeeping

The PR bundles the AITADS fixtures, scorecards, eval harness and closure measurement with the safety fix. Splitting them would make the fix easier to review and revert.

[claude]

Review (design-level)

Scope note: GitHub can't return this PR's diff (over 300 files) and the checkout is shallow. I read domain/verdict_key.py, core/verdict_cache.py and migration 0016 in full. I did not read the rest (services, API, eval, frontend) and did not run the tests.

What looks good

• Fail-closed design: verdict_key returns None on truncation, oversized values, too many fields, model-normalised alerts and alerts with no anchor. closing_decisions treats an unreadable threat time as a block.
• Request-controlled fields (user agent, URL, O365 operations, GrantedAccess) go into the key but can't create one.
• Values are compared in full, with no masking or truncation.
• The two-analyst rule (one Redis hash entry per decided_by) dedupes group and bulk decisions, and MAX_DECISIONS bounds the hash.
• Tombstones on both key and fingerprint, and old key versions are never read.
• Migration 0016 audits each disabled rule in the tenant's hash chain under the chain lock. The gate also ignores fingerprint-only rules, so a manual re-enable stays harmless.
• The acceptance test (0 of 208 malicious closes), the revocation tests and the migration test cover the right things.

Concerns and suggestions

1. PR size and mixed scope: it carries unmerged AI-E1.9 ([AI-E1.9] Auto-close calibration: benign alerts should close without weakening the zero-miss bound #316) and fatigue-1 work, re-recorded cassettes, scorecards, an aitads fixture and an eval oracle. That is hard to review and bisect. Consider landing the key, cache, suppression and migration separately from the eval tooling, or say which commits hold the security-relevant change.
2. Migration 0016 collides with AI-E1.5c+d: shadow SELECT-only DB role, separate LLM slots, gate and F6 #323. A note there or a CI check would keep the renumbering from slipping.
3. Residual L2: a rule whose regex exhausts the work budget isn't revoked, so it keeps closing alerts after a confirmed threat. Consider failing safe (disable the rule or flag it for admin review) rather than only logging and counting.
4. The fingerprint-wide tombstone means one true_positive blocks cache closes for every key under that fingerprint for 7 days. It is safe but worth documenting as intended.
5. verdict_key._walk yields only str and int, so a float in a behaviour field is silently dropped and two alerts can share a key. Include floats or treat them as unreadable.
6. TRUNCATED_MARKER and LLM_NORMALIZED_FLAG are copied to satisfy import-linter. A unit test asserting they equal the originals would catch drift that would silently disable the truncation guard.
7. Closure cost: 82 down to 11 of 312 benign closes is justified by the safety gain, but a 7-day TTL with a 2-analyst minimum may leave the cache idle in small tenants. Consider a per-tenant MIN_ANALYSTS with a hard floor of 2.

Security
No new concerns beyond the above. clear_tenant escapes [ and ] in its Redis pattern but not * or ?, so confirm tenant_id is validated at the API layer.

Overall this is a well-reasoned fix. My main requests are to split the PR if possible and to close or fail-safe the L2 revocation gap.