feat(scanner): add the evaluate() contract foundation (#369)

[parthrohit22]

What does this PR do?

Adds the foundation for migrating scanner rules to the evaluate() coverage contract: inventory calls that report failure, shared evaluation helpers, a single engine path for migrated rules, and a contract test every migrated rule must pass.

Changes

• scanner/azure_client.py: new list_storage_accounts() and list_key_vaults() return None when the list call fails, so evaluate() can tell a failure from an empty subscription. get_storage_accounts() / get_key_vaults() wrap them and still return [], so legacy scan() rules are unchanged.
• scanner/evaluation.py: standard reason codes (INVENTORY_UNAVAILABLE, NO_RESOURCES_FOUND, EVIDENCE_UNAVAILABLE, MISSING_PROPERTIES, POLICY_NOT_REQUIRED, APPROVED_EXCEPTION) and helpers inventory_unavailable(), no_resources_found() and fail_findings().
• scanner/engine.py:
  • A rule exposing evaluate() is run through evaluate() only, and its findings come from its FAIL evaluations. This removes duplicate Azure list calls and the risk of scan() and evaluate() drifting apart.
  • A rule is listed in failed_rule_ids when its evaluate() crashes, returns malformed data, or reports any ERROR evaluation (for example INVENTORY_UNAVAILABLE), so failed_rule_ids and the evaluations always agree. A per-resource gap is UNKNOWN, not ERROR, and does not list the rule.
  • Items that are not RuleEvaluation objects are dropped individually instead of failing result serialisation for the whole scan. The rule's valid evaluations and FAIL findings are kept, and one ERROR / MALFORMED_EVALUATION row names the malformed positions.
  • A FAIL without a finding is logged.
• scanner/rules/az_kv_006.py: a failed vault list reports ERROR / INVENTORY_UNAVAILABLE instead of NOT_APPLICABLE. scan() is now a thin wrapper over evaluate(), and evaluations record enable_rbac_authorization as evidence. Findings are unchanged.
• tests/test_rule_evaluation_contract.py (new): discovers every rule exposing evaluate() and checks that:
  • a failed inventory is ERROR, never PASS / NOT_APPLICABLE;
  • an empty inventory is NOT_APPLICABLE;
  • every FAIL carries the rule's finding;
  • scan() returns exactly the FAIL findings.
    Each migrated rule must register a fixture. Against the previous AZ-KV-006 this test fails, as intended.
• docs/adding-a-rule.md: new rules should implement evaluate(), with the reason-code table and a template.
• CHANGELOG.md: Added / Changed / Fixed entries.

Type of change

• New scan rule
• Remediation playbook
• Bug fix
• Dashboard/front-end work
• API endpoint
• Documentation
• Compliance mapping

Also a scanner engine change (evaluate() contract foundation).

Rule details (if applicable)

• Rule ID: AZ-KV-006 (behaviour on inventory failure only; detection unchanged)
• Severity: MEDIUM (unchanged)
• Category: Key Vault
• Frameworks mapped: unchanged

Testing

• Tested against a real Azure free trial subscription (not needed: no detection logic changed; covered by offline mocks)
• Returns correct JSON output
• All seven CI checks pass (pending CI)
• No hardcoded credentials or secrets

Local results:

• New and updated suites pass: test_rule_evaluation_contract, test_rule_evaluations, test_rules_keyvault, test_azure_client_management, test_engine_integration, test_compliance_scoring, test_rules_storage, test_pqc_rules.
• ruff check . and ruff format --check . are clean.
• The full suite shows the same set of failures before and after this change on a machine without Postgres. Those are the Postgres-backed tests that CI runs against its ephemeral database.

Related issue

Closes #369

Part of #380. Unblocks #370 and the good first issues #376, #377, #378 and #379.

Checklist

• Every commit includes a DCO Signed-off-by trailer (git commit -s; see docs/dco.md)
• My code follows the rule template in CONTRIBUTING.md
• I added or updated the matching CLI playbook (not applicable: no remediation change)
• I added or updated all four compliance framework mappings (not applicable: mappings unchanged)
• I have not committed any real Azure credentials
• My branch name follows the convention: feat/description

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[TFT444]

The foundation is solid and the design is well thought through. One item needs addressing before merge.

Warning: failed_rule_ids does not include rules that return ERROR/INVENTORY_UNAVAILABLE

scanner/engine.py:260-271: when evaluate() returns an ERROR/INVENTORY_UNAVAILABLE evaluation (the Azure list call failed and returned None), _run_evaluate marks the rule as completed and does NOT append it to failed_rule_ids. A consumer reading failed_rule_ids alone (for example get_compliance_score()) would treat that rule as having run cleanly, even though it never inspected any resources.

Please either:

1. Append to failed_rule_ids when any returned evaluation has status == EvaluationStatus.ERROR, so the two signals stay consistent; or
2. Add a clear docstring/comment at failed_rule_ids stating it only tracks evaluator crashes, and that ERROR evaluations must be read from evaluations directly, and update get_compliance_score() to check both.

Everything else looks good: backward compat on get_* vs list_*, the exclusive dispatch guard, FAIL-without-finding logging, _run_evaluate isinstance validation, and the contract test invariants are all correct. Happy to re-review once the above is addressed.

[parthrohit22]

@TFT444 Thanks for the review. Fixed in 5b132cc, using option 1: a rule is now added to failed_rule_ids whenever evaluate() crashes, returns malformed data, or reports any ERROR evaluation (e.g. INVENTORY_UNAVAILABLE). The comment on failed_rule_ids states this, and two engine tests cover an inventory ERROR and a mixed PASS + ERROR result.

For context on scoring: get_compliance_score() already reads the rule_evaluations rows as well as failed_rule_ids (api/models/finding.py, around lines 1795–1861), and an ERROR row rolls up to ERROR (tests/test_compliance_scoring.py::test_persisted_unknown_and_error_evaluations_are_reported_not_promoted), so no score was affected. This change keeps the two signals consistent. Ready for re-review.

[TFT444]

Thanks for the quick fix on failed_rule_ids / ERROR semantics. Two more issues on the updated head before this is ready.

1. _run_evaluate all-or-nothing validation silently discards valid evaluations

scanner/engine.py:287-291

The item validation loop raises TypeError on the first non-RuleEvaluation item, which causes the except block to discard the entire list and emit a single EVALUATOR_EXCEPTION ERROR. If evaluate() returns [valid_eval, valid_eval, bad_dict], the two valid evaluations are thrown away and the rule is reported as completely broken. That misrepresents what the rule actually found.

Please either filter and log bad items individually (keeping the valid ones), or validate the full list before processing it and surface exactly which item was malformed. The contract test only ever feeds a fully malformed list, so the mixed case is not currently covered.

2. any(ERROR) in failed_rule_ids overstates rule failures

scanner/engine.py:199

A rule that evaluated 8 resources successfully and got EVIDENCE_UNAVAILABLE on 2 is listed in failed_rule_ids alongside rules that completely crashed. A caller reading failed_rule_ids (for example a compliance dashboard) would treat it as "rule did not complete" even though it did most of its job.

Please either raise the threshold (e.g. only list the rule when ALL evaluations are ERROR, or when the rule crashed entirely), or add a clear docstring on failed_rule_ids in the scan result schema stating it means "at least one ERROR evaluation occurred" so callers know not to treat it as a binary completed/failed signal. If the intent is truly to flag any partial failure, that decision should be explicit and tested.

[parthrohit22]

Hi @TFT444 - I have pushed the requested fixes and would appreciate a re-review. evaluate() now preserves valid RuleEvaluation results when the returned list also contains malformed items, while recording a MALFORMED_EVALUATION error with the bad positions. failed_rule_ids now distinguishes partial usable coverage from rules with no usable evaluations, and the behavior is documented and tested. The latest commit has the required DCO sign-off. Targeted tests: 87 passed; Ruff and git diff --check passed. CI is green .

[ritiksah141]

Approving. Reviewed the current head (3827e15) locally: full diff read, based on the current dev tip, both items from the last review round fully addressed and tested (malformed items are dropped individually while valid evaluations and findings survive, and failed_rule_ids now uses the stricter all-ERROR semantics with the decision documented and explicitly tested). The eight named suites pass 175/175, the full suite passes with 1537 passed / 93 skipped / 0 failed, and ruff is clean.

One small ask before merge: the CHANGELOG "Changed" entry still says a rule reporting "at least one ERROR evaluation" is listed in failed_rule_ids, which contradicts the final all-ERROR behaviour. Suggested replacement for that sentence:

"A rule whose evaluate() crashes, returns malformed data, or returns only ERROR evaluations (such as an inventory-wide INVENTORY_UNAVAILABLE failure with no usable outcomes) is listed in failed_rule_ids; a partial ERROR alongside usable outcomes does not list the rule, and a per-resource gap is UNKNOWN."

Otherwise this is a clean foundation for #380 and good to land.