feat: persist PASS/FAIL/ERROR/NOT_APPLICABLE per rule per resource, fix compliance score

[parthrohit22]

What problem does this solve?

Two coupled problems, one fix:

1. The scanner only persists failing findings. scanner/engine.py::run_scan() collects a flat
   findings list — there is no record of which rules ran clean, which errored, or which
   resources were never evaluated.
2. As a direct consequence, api/models/finding.py::get_compliance_score() computes control
   status as status = "FAIL" if rule_id in failed_rule_ids else "PASS" — any rule that errored,
   whose resource was never observed, or that never ran is counted as PASS by default. This
   inflates every reported compliance score.

Both block every downstream reasoning-layer feature (drift history, campaign detection,
attack-path confidence) and the eight already-open "enterprise rules" issues (#253, #255, #257,
#258, #259, #261, #262, plus closed #256/#260) that already assume a PASS/FAIL/UNKNOWN/
NOT_APPLICABLE contract exists, with no shared engine support for it.

PR #250 ("Azure Resource Graph inventory foundation") already lands Phase 1 of the underlying
evidence model — normalised, tenant-scoped resource snapshots with complete/partial/failed
collection state — but explicitly does not persist rule-outcome data. This issue is Phase 2: the
rule-outcome side, built on top of #250's snapshots.

Describe the solution

• New table (e.g. rule_evaluations) keyed on (scan_id, rule_id, resource_id) with a status
  enum: PASS / FAIL / ERROR / NOT_APPLICABLE, plus a free-text reason for ERROR/NOT_APPLICABLE.
• ScanEngine.run_scan() writes an evaluation row for every resource a rule was applicable to,
  not just the ones that failed.
• Update get_compliance_score() to compute PASS only from persisted successful evaluations in
  this table — remove the absence-implies-PASS default entirely.
• Document the contract in docs/adding-a-rule.md so new rules (including the 8 open enterprise
  issues) target it directly instead of inventing ad hoc indeterminate handling (see
  scanner/rules/az_cmp_002.py's own LOW-severity indeterminate workaround for what happens
  without a shared model).

Alternatives considered

Ship the schema and the compliance-score fix as two separate PRs — rejected; the fix has no
correct implementation without the schema existing first, and reviewing them apart just means
merging a known-broken intermediate state.

Additional context

Depends on: PR #250 (merge first). Blocks: issue 2 below, and all eight open enterprise-rules
issues. Exit test: a forced API error or unseen resource never appears as PASS.

Coordination

This issue is the shared evaluation-contract foundation for the open enterprise-rule packs. It
does not duplicate them — it provides the engine support (per-resource PASS / FAIL / ERROR /
NOT_APPLICABLE) they assume but do not themselves build. It explicitly blocks and should be
linked from: #253, #254, #255, #257, #258, #259, #261, #262. Enterprise-rule PRs should consume
this persisted evaluation contract instead of re-implementing ad hoc indeterminate handling.

[parthrohit22]

Coordination note — foundation for the enterprise-rule packs (not a duplicate).

This issue is the shared rule-evaluation contract the open enterprise-rule packs depend on. It provides the per-resource PASS / FAIL / ERROR / NOT_APPLICABLE persistence that those packs assume but do not themselves build. It does not duplicate any enterprise-rule issue; rather, it explicitly blocks and should be linked from:

#253, #254, #255, #257, #258, #259, #261, #262

Enterprise-rule PRs should consume this persisted evaluation contract instead of re-implementing ad hoc indeterminate handling. Added a "Coordination" section to the issue body for reference.

[dipeshrayg]

I'd like to take this on. Before writing code, here's the design I'm planning, since the resource-level part isn't fully mechanical from the current rule contract.

The core problem: today, every rule's scan() only returns a findings list (violations). The engine has no visibility into which resources a rule actually examined, so there's no denominator for a true per-resource PASS, and no way to distinguish "ran clean" from "never ran" or "not applicable."

The plan: a new Alembic migration adds a rule_evaluations table:

rule_evaluations
  id, scan_id (FK scans), rule_id, resource_id, resource_type,
  status (PASS | FAIL | ERROR | NOT_APPLICABLE),
  reason (nullable, populated for ERROR/NOT_APPLICABLE),
  evaluated_at

Each rule gets one new module constant, RESOURCE_TYPE, declared statically (most rules already embed this string in their finding dicts on FAIL, this just makes it available up front instead of only on failure). ScanEngine.run_scan() cross-references each rule's RESOURCE_TYPE against the subscription's resource inventory from PR #250's InventorySnapshot once it's merged: findings become FAIL, applicable resources with no finding become PASS, an exception during scan() becomes ERROR for the whole applicable set, and a resource type absent from the inventory becomes NOT_APPLICABLE. Rules that haven't declared RESOURCE_TYPE yet keep working exactly as they do today, rule-level, absence-implies-PASS, with a logged warning, so nothing breaks and no 71-rule rewrite is required up front. Rules migrate incrementally. get_compliance_score() reads from rule_evaluations for migrated rules and falls back to today's logic for the rest until migration completes.

First PR scope: the migration, the engine wiring, the get_compliance_score() rewrite, and RESOURCE_TYPE added to a small reference set of rules (the two I've already authored), not a full 71-rule migration in one PR.

Question for maintainers: should NOT_APPLICABLE resolve against PR #250's ARG inventory specifically, or against the existing per-service AzureClient collectors directly?

[m-khan-97]

Thanks for volunteering to take this on. The overall direction makes sense, but I want us to adjust a few parts of the design before implementation so we do not carry the current scoring bug into the new model.

• We should not keep the old “absence means PASS” fallback for rules that have not migrated yet. That is the main defect this issue is meant to remove. If a rule has not produced enough evidence for a real PASS, it should remain UNKNOWN or ERROR, or be explicitly marked NOT_APPLICABLE.
• RESOURCE_TYPE plus the ARG inventory is not enough by itself to decide whether a rule applies. Applicability may depend on tags, policy, child resources, service tier, or evidence that ARG does not contain. I would prefer the rule contract to return typed evaluation results for the resources it actually assessed, including the reason and evidence behind PASS, FAIL, ERROR, or NOT_APPLICABLE.
• PR feat(scanner): add Azure Resource Graph inventory foundation #250 can provide the discovery baseline when its inventory is complete. Please preserve its complete, partial, and failed states: partial or failed discovery must not be used to infer NOT_APPLICABLE. Targeted collectors can add evidence, but collector failures should remain UNKNOWN or ERROR.
• The schema migration and compliance-score correction should land together. Please include tests proving that rule exceptions, missing evidence, partial inventory, and unmigrated rules never improve the reported score.

Could you post a revised schema and rule-result contract here before opening the implementation PR? That will let the leads agree on the denominator and migration behavior early.

I also tried to assign the issue to you, but GitHub currently reports your account as ineligible for assignment. A repository owner may need to restore or grant triage access first.

[ritiksah141]

@dipeshrayg, @m-khan-97, I reviewed the proposed design against the current scanner engine, database persistence, the merged ARG inventory foundation in #250, the enterprise rule work including #293, and the compliance reporting requirements in #302.

The main direction is correct, but before implementation I think the contract should be revised in the following areas so we do not carry the existing absence-means-PASS behavior into the new model.

1. Keep UNKNOWN separate from ERROR

The issue currently refers to a PASS / FAIL / UNKNOWN / NOT_APPLICABLE contract, while the proposed table uses PASS / FAIL / ERROR / NOT_APPLICABLE. UNKNOWN and ERROR represent different conditions and should not be collapsed:

• PASS: complete evidence explicitly proves the resource is compliant.
• FAIL: complete positive evidence proves a violation.
• UNKNOWN: evidence is unavailable, partial, inaccessible, stale, or insufficient to make a decision.
• ERROR: the rule evaluator itself failed because of an internal code/runtime error.
• NOT_APPLICABLE: complete evidence explicitly proves the rule does not apply to the resource or scope.

This distinction is important operationally. Missing Azure permissions or an unreachable Kubernetes API is not the same as a rule implementation crashing, and both need different remediation and observability.

2. Use a typed rule-evaluation result

I do not think RESOURCE_TYPE plus ARG inventory is sufficient to infer PASS or applicability. Applicability can depend on SKU, tags, child resources, policy, configuration, or targeted SDK/Kubernetes evidence that ARG does not contain.

The rule should return the resources/scopes it actually evaluated and the resulting status, for example:

RuleEvaluation(
    rule_id="AZ-AKS-019",
    resource_id="...",
    resource_type="Kubernetes/workloads",
    status=EvaluationStatus.UNKNOWN,
    reason_code="CLUSTER_UNREACHABLE",
    reason="Kubernetes API could not be reached",
    evidence={
        "source": "Kubernetes API",
        "collected_at": "...",
        "confidence": "UNKNOWN",
    },
    finding=None,
)

A FAIL evaluation can carry or reference its finding. This lets the engine persist PASS, FAIL, UNKNOWN, ERROR, and NOT_APPLICABLE without trying to reverse-engineer them from a flat findings list.

3. Suggested schema

The rule_evaluations table should include at least:

• scan_id with a foreign key to scans;
• rule_id;
• resource_id and resource_type;
• constrained status;
• stable reason_code plus safe free-text reason;
• evidence/provenance JSONB;
• evaluated_at;
• a uniqueness constraint on (scan_id, rule_id, resource_id).

Evaluation and finding persistence should occur in the same transaction and be idempotent when a scan is retried. The schema migration, persistence path, and compliance-score correction should land together so there is no intermediate release that still reports missing evaluations as PASS.

4. Do not retain a legacy absence-means-PASS fallback

For rules not yet migrated to the typed result contract, absence must not be treated as PASS. That would preserve the exact defect #263 is fixing.

During migration, an unmigrated rule should create a rule/scope-level UNKNOWN result such as LEGACY_RULE_NOT_MIGRATED, or otherwise be explicitly reported as unevaluated. It must not contribute to verified PASS counts. This still allows incremental migration without requiring all existing rules to be rewritten in the first PR.

5. ARG inventory is a discovery baseline, not the applicability authority

The #250 snapshot states need to remain authoritative about collection completeness:

• COMPLETE can support PASS or NOT_APPLICABLE only when all evidence required by that rule is present.
• PARTIAL must produce UNKNOWN for affected resources/scopes and must never be used to infer absence.
• FAILED must produce UNKNOWN for the requested scope.
• Targeted SDK or Kubernetes collectors may enrich the ARG baseline, but their failures must also remain UNKNOWN.

An empty result from a complete collector may prove NOT_APPLICABLE; an empty result from a failed or partial collector cannot.

6. Compliance aggregation and denominator

At framework-control level, I suggest the following conservative aggregation order:

1. Any explicit FAIL -> FAIL.
2. Otherwise, any evaluator ERROR -> ERROR.
3. Otherwise, any UNKNOWN/incomplete required scope -> UNKNOWN.
4. Explicit applicable evaluations all passing -> PASS.
5. Explicitly inapplicable across a complete scope -> NOT_APPLICABLE.

Reports should expose passed, failed, unknown, errors, not-applicable, and evaluated coverage separately. UNKNOWN and ERROR must not improve the reported score, and NOT_APPLICABLE should be excluded from the technical-evidence denominator. This also prepares the data model for #302, where the output becomes evidence coverage rather than a certification claim.

7. Required regression coverage

The implementation should prove at least these cases:

• a rule exception never appears as PASS;
• missing permissions or collector failure becomes UNKNOWN;
• partial ARG/Kubernetes inventory cannot infer PASS or NOT_APPLICABLE for missing scopes;
• a complete empty inventory can produce NOT_APPLICABLE;
• an unmigrated rule never contributes to PASS;
• mixed per-resource results aggregate conservatively;
• retries do not duplicate evaluations/findings;
• evaluation and finding writes roll back together;
• compliance PASS is emitted only from explicit successful evaluations.

For #293 specifically, this contract will allow unreachable clusters, failed namespaces, and missing policy to persist as UNKNOWN; policy-excluded namespaces as NOT_APPLICABLE; secure assessed workloads as PASS; and violations as FAIL linked to their findings.

If we agree on these points, I suggest posting the final dataclass/enum and migration shape here first, then implementing one reference rule end to end before migrating the wider rule set. That should keep the initial PR reviewable while still removing absence-means-PASS from day one.

@dipeshrayg, also if you are onto that then please respond as some of other rule based PR depend on this.

[dipeshrayg]

Revised design incorporating both reviews. The original proposal had two real gaps: collapsing UNKNOWN into ERROR, and inferring applicability from ARG inventory instead of asking each rule what it actually evaluated. Fixing both here.

Status enum, five states:

class EvaluationStatus(str, Enum):
    PASS = "PASS"
    FAIL = "FAIL"
    UNKNOWN = "UNKNOWN"
    ERROR = "ERROR"
    NOT_APPLICABLE = "NOT_APPLICABLE"

PASS and NOT_APPLICABLE require complete evidence. UNKNOWN covers missing permissions, partial or failed collection, and unmigrated rules. ERROR is reserved for the evaluator itself raising, a code or runtime failure, not a data-availability problem.

Rule contract, additive, not a rewrite of the 71 existing rules: a rule opts in by defining a new evaluate(azure_client, subscription_id) -> List[RuleEvaluation] function alongside its existing scan().

@dataclass
class RuleEvaluation:
    rule_id: str
    resource_id: str
    resource_type: str
    status: EvaluationStatus
    reason_code: Optional[str] = None
    reason: Optional[str] = None
    evidence: Dict[str, Any] = field(default_factory=dict)
    finding: Optional[Dict[str, Any]] = None

A FAIL evaluation carries its finding in the same object, so evaluation and finding are written in one transaction, never two separate writes that can drift apart on a retry.

Rules that only define scan() (everything not yet migrated) get exactly one rule-level evaluation per run: UNKNOWN, reason_code="LEGACY_RULE_NOT_MIGRATED". No fallback to PASS, ever. This is what actually removes the defect instead of relocating it.

Schema:

rule_evaluations
  id, scan_id (FK scans), rule_id, resource_id, resource_type,
  status, reason_code, reason, evidence (JSONB), evaluated_at
  unique (scan_id, rule_id, resource_id)

ARG inventory's role: discovery baseline only, never the applicability authority. A rule's own evaluate() is what decides PASS, FAIL, UNKNOWN, or NOT_APPLICABLE for the resources it examined. InventorySnapshot.status from #250 is used only as a safety check: if ARG's snapshot for a resource type is COMPLETE and it lists a resource the rule's evaluate() didn't cover at all, the engine records that gap as UNKNOWN too, catching a rule that silently skipped something. If ARG itself is PARTIAL or FAILED for that scope, the engine skips this cross-check entirely rather than guessing. This is a defense-in-depth addition on top of the rule's own report, not a replacement for it, and it stays out of the first PR to keep scope tight, the rule contract and legacy fallback are the load-bearing part.

Compliance aggregation, applied per rule per resource then rolled up to control level:

1. Any FAIL, control is FAIL.
2. Else any ERROR, control is ERROR.
3. Else any UNKNOWN, control is UNKNOWN.
4. Else all applicable evaluations PASS, control is PASS.
5. Else everything NOT_APPLICABLE, control is excluded from the scored denominator.

The report exposes passed, failed, unknown, errors, and not-applicable counts separately instead of collapsing them into one percentage.

First PR scope: the migration, the RuleEvaluation/EvaluationStatus contract, the engine wiring for both the new evaluate() path and the legacy fallback, the get_compliance_score() rewrite, and one rule migrated end to end as the reference implementation (AZ-KV-006, it's simple enough to keep the PR reviewable while still exercising the full path including a FAIL, a PASS, and an UNKNOWN case). No other rule gets touched in this PR.

Regression coverage, all nine scenarios from the review:

• a rule exception never appears as PASS
• missing permissions or collector failure becomes UNKNOWN
• partial inventory cannot infer PASS or NOT_APPLICABLE for missing scopes
• a complete empty inventory can produce NOT_APPLICABLE
• an unmigrated rule never contributes to PASS
• mixed per-resource results aggregate conservatively
• retries do not duplicate evaluations or findings
• evaluation and finding writes roll back together
• compliance PASS is emitted only from explicit successful evaluations

One open question I can't resolve without a maintainer call: on assignment, GitHub's telling m-khan-97 my account is ineligible. Not blocking anything so far since I've been working through comments and a fork PR the same way as the other contributions, flagging it in case a repo owner wants to fix it.

Let me know if this matches what you both had in mind, then I'll open the PR against this scope.

[m-khan-97]

@dipeshrayg, the revised direction matches what we need and you can proceed with the first implementation PR after #308 merges and you rebase onto its Alembic head.\n\nPlease carry these final contract conditions into the PR:\n\n- Keep resource_id non-null and define a canonical scope identifier for subscription/rule-level UNKNOWN or ERROR results; do not use an empty string as identity.\n- Persist an explicit durable link between a FAIL evaluation and its finding (for example a nullable finding_id foreign key populated in the same transaction), so reports never have to infer the relationship.\n- Add database CHECK constraints for the five statuses and require stable reason_code values for UNKNOWN, ERROR, and NOT_APPLICABLE.\n- Legacy scan-only rules may continue producing findings, but their evaluation coverage must remain UNKNOWN/LEGACY_RULE_NOT_MIGRATED and must never contribute a PASS.\n- Evaluator exceptions must create an ERROR result at a canonical rule/scope level even when no resource inventory is available.\n- Version the API/report response and expose evaluated coverage plus passed, failed, unknown, error, and not-applicable counts separately. UNKNOWN and ERROR must never improve a score.\n- Add a single-head Alembic assertion and a populated-database migration test. The migration must be rebased and chained after #308 rather than sharing its current parent.\n- Keep the first PR to the contract, persistence/aggregation path, and one end-to-end reference rule as proposed. The ARG completeness cross-check can remain a separately tracked follow-up.\n\nWith those conditions, please open the implementation as a draft PR and link #263. Include the nine regression scenarios from your revised design in its checklist.

[dipeshrayg]

Understood, carrying all eight conditions into the implementation:

• resource_id stays non-null. Subscription/rule-level UNKNOWN or ERROR results get a canonical scope identifier instead of an empty string.
• FAIL evaluations get a nullable finding_id foreign key, populated in the same transaction as the finding write.
• CHECK constraints on the five statuses, NOT NULL reason_code required when status is UNKNOWN, ERROR, or NOT_APPLICABLE.
• Legacy scan-only rules stay UNKNOWN/LEGACY_RULE_NOT_MIGRATED, never contribute a PASS.
• Evaluator exceptions produce an ERROR at the canonical rule/scope level even with no resource inventory available.
• Versioned API/report response exposing evaluated, passed, failed, unknown, error, and not-applicable counts separately. UNKNOWN and ERROR never improve the score.
• Single-head Alembic assertion plus a populated-database migration test, chained after fix(core): enforce severity contract v1 #308's revision.
• Scope stays exactly what I proposed: contract, persistence/aggregation, and AZ-KV-006 as the one reference rule. The ARG completeness cross-check stays a separate follow-up.

Waiting for #308 to merge before opening the draft PR, since the migration needs to chain onto its actual revision head, not the current tip. I'll open the draft PR linking #263 with the nine regression scenarios in the checklist as soon as that lands.