feat(engine): add rule evaluation coverage contract (#263) - #321
Conversation
|
@dipeshrayg, this remains the canonical review track for #263’s evaluation semantics: status vocabulary, per-resource evidence rows, reason-code constraints, finding linkage, aggregation, and the reference-rule migration. Please keep it draft while we align its migration and persistence API with #325. Do not widen it into leases, admission, enrichment, or finding lifecycle; those belong to separate layers. |
|
Understood, keeping this in draft and staying within the evaluation-semantics scope you outlined. No leases, admission, enrichment, or finding lifecycle here. I looked at #325 to see where the overlap is concretely: its I'll hold off on touching the migration or persistence code here until you've decided how the two should line up. Happy to adjust either the migration chain or the persistence approach once that's settled, whichever way you want it to land. |
|
Cross-PR coordination note: #325 (scan leases/fencing/idempotency, #303) also creates a rule_evaluations table and its own persistence semantics, which overlaps this PR. Per m-khan, this PR is the agreed #263 evaluation-contract implementation, so #325 should drop its copy and keep only the fencing/idempotency layer. Sharing the side-by-side here so the reconciliation is clear on both sides. Schema: rule_evaluations
The core columns, keys, and constraints overlap and share names, so whichever migration runs second fails with "relation already exists." This PR is the strict superset: it adds the rule_id and status indexes and the reason_code-required CHECK. Producer (who emits evaluations)
Only this PR makes evaluations observable; #325 has no producer, so its evaluations would always be empty in production. save_scan persistence semanticsBoth PRs are full rewrites of the same method with incompatible idempotency models, so the conflict is larger than the table.
Compliance score (the actual #263 bug)
Only this PR fixes the score-inflation bug. Recommended resolution
One integration detail for whoever reconciles: this PR's engine adds evaluate()-derived FAIL findings to the findings list, and #325 derives finding_key from rule_id plus resource scope plus discriminator. Those compose, but make sure evaluate()-derived findings get stable finding_keys so the upsert stays idempotent. |
|
Thanks for the detailed side by side, that matches what I found and goes further. Agreed with the recommended resolution: this PR owns the schema, scanner/evaluation.py, the engine producer, aggregation, and the compliance-score fix, #325 keeps leases/fencing/admission/enrichment/metrics and drops its rule_evaluations copy. On the save_scan reconciliation: agreed the real merge point is there, not just the table, and that the final version should be #325's fenced upsert skeleton with this PR's evaluation field set and FAIL-to-finding_id linkage folded in, using upsert-plus-delete-absent for evaluations to match #303's replay-safety model rather than this PR's current per-scan delete-and-reinsert. I'll hold off implementing that here until the ordering is confirmed, since it depends on #325's lease/fencing columns and finding_key concept that don't exist in this PR's schema. Also agreed on the finding_key point for evaluate()-derived findings, once that reconciliation happens I'll make sure AZ-KV-006's FAIL findings produce a stable, discriminator-safe key so the upsert in #325's model stays idempotent. Ready whenever the merge order is decided. |
|
Ordering proposal for this PR and #325. My suggestion: merge this PR (#321) first as the canonical evaluation contract, then rebase #325 on top of it. #325 would drop its duplicate rule_evaluations copy and re-chain its migrations on top of 3f59f83a5253, keeping its leases, fencing, admission, enrichment, and metrics work and folding this PR's evaluation writes into its fenced save_scan. This matches the resolution agreed above. Two reasons I want to confirm the direction now. @SHAURYAKSHARMA24 does not seem active on #325 at the moment, and my own draft PR #293 also depends on this resolution landing, since it needs the canonical contract in place before it can move forward. @m-khan-97 can you approve the ordering, #321 first? Once you confirm, @dipeshrayg this PR would be clear to make the save_scan change we discussed (upsert plus delete-absent for evaluations) and get ready to merge. Happy to help drive it however is useful. |
Resolves the two must-fix items and the follow-ups raised in review of OWASP#325. Integration: leave the OWASP#263 evaluation contract to OWASP#321 ------------------------------------------------------ This branch created a second `rule_evaluations` table, near-identical to the one PR OWASP#321 adds, that no production code ever populated: `run_scan()` emits no `evaluations` key, so the storage, the upsert and its tests described a contract that could not be observed in production. Two `CREATE TABLE rule_evaluations` statements would also have broken whichever of OWASP#321/OWASP#325 merged second, independently of the Alembic head ordering. Issue OWASP#263 owns that contract - the table, the PASS/FAIL/UNKNOWN/ERROR/ NOT_APPLICABLE semantics, engine emission and the compliance-score fix - so this branch drops it entirely and keeps only what OWASP#303 asks for: the stable `finding_key` identity and its unique index. `save_scan()` marks where OWASP#321's evaluation writes belong, inside the fenced completion transaction, so they inherit the lease/ownership check without re-implementing it. Terminally failed enrichment jobs are recoverable again ------------------------------------------------------- After three failed attempts a job became `failed` and nothing could move it: enqueue used ON CONFLICT DO NOTHING, claim only selected `pending`, and stale recovery only handled expired `running` leases. That regressed the operator retry the old thread-based path gave for free. `enqueue_enrichment_job()` now returns an explicit outcome - `created`, `requeued`, `active` or `completed` - and atomically resets a `failed` job to `pending` with a fresh retry budget. It keeps the same job row, its last error message (audit) and its checkpoint (so the retry resumes), never revives a `completed` job, and never disturbs a live `running` lease. The conditional UPDATE is the whole guard, so concurrent re-POSTs converge on one logical job. Admission migration cannot leave an INVALID index ------------------------------------------------- `CREATE UNIQUE INDEX CONCURRENTLY uq_scans_one_active_per_subscription` fails, and leaves an unusable index behind, on a deployment that already holds several active scans for one subscription. The migration now preflights, changes nothing, and names the offending subscriptions in an actionable error; deciding which production scan is authoritative stays an operator call, and no scan history is deleted. A retry is safe: an INVALID index from an interrupted build is dropped before rebuilding. PostgreSQL tests no longer race the shared queue ------------------------------------------------ Fixtures called `claim_next_pending_scan()`, which takes the globally oldest pending scan, then persisted against the scan they had just created - so any unrelated pending row made `save_scan()` raise LostLease. Reproduced on a real database: with one older pending scan present, the old fixture claims someone else's row and fails; the new one does not. `claim_next_pending_scan()` and `claim_next_enrichment_job()` take an optional `scan_id` so a caller can claim a known row under identical lease and fencing semantics, and the fixtures use it. Queue-wide `recover_stale_*()` counts are asserted as progression of the test's own row rather than as global totals. Follow-ups ---------- - worker_heartbeats is bounded: rows past WORKER_HEARTBEAT_RETENTION_SECONDS are pruned on the beat that registers a new worker identity - once per process, not on every beat, and never for a live worker. - The scan and enrichment queues alternate one item per loop iteration instead of draining enrichment first, so neither can starve the other. - Added the partial index that keeps /metrics' last-successful-scan lookup from degrading into a sequential scan as history grows. - Documented SCAN_LEASE_SECONDS, SCAN_HEARTBEAT_SECONDS, OPENSHIELD_MAX_SCANS_PER_SUBSCRIPTION_PER_HOUR and WORKER_HEARTBEAT_RETENTION_SECONDS in .env.example. - docs/api-reference.md now documents the real admission and enrichment contracts, including which status codes are actually returned. Tests ----- New PostgreSQL coverage for terminal-failure requeue (explicit requeue, no restart of a completed job, a live lease is not stolen, concurrent requeues converge, a stale token cannot write after reclaim, a requeued job completes), the duplicate-active-scan migration preflight, stale heartbeat pruning, and worker fairness with both queues backlogged. Full suite on postgres:16-alpine: 897 passed, 2 skipped. The one failure, test_vector_store_purity, is a local checkout artifact - it needs a BM25 index this checkout's ai/vectorstore lacks, and passes in a clean worktree. Signed-off-by: Shaurya K Sharma <shauryaksharma24@gmail.com>
Resolves the two must-fix items and the follow-ups raised in review of OWASP#325. Integration: leave the OWASP#263 evaluation contract to OWASP#321 ------------------------------------------------------ This branch created a second `rule_evaluations` table, near-identical to the one PR OWASP#321 adds, that no production code ever populated: `run_scan()` emits no `evaluations` key, so the storage, the upsert and its tests described a contract that could not be observed in production. Two `CREATE TABLE rule_evaluations` statements would also have broken whichever of OWASP#321/OWASP#325 merged second, independently of the Alembic head ordering. Issue OWASP#263 owns that contract - the table, the PASS/FAIL/UNKNOWN/ERROR/ NOT_APPLICABLE semantics, engine emission and the compliance-score fix - so this branch drops it entirely and keeps only what OWASP#303 asks for: the stable `finding_key` identity and its unique index. `save_scan()` marks where OWASP#321's evaluation writes belong, inside the fenced completion transaction, so they inherit the lease/ownership check without re-implementing it. Terminally failed enrichment jobs are recoverable again ------------------------------------------------------- After three failed attempts a job became `failed` and nothing could move it: enqueue used ON CONFLICT DO NOTHING, claim only selected `pending`, and stale recovery only handled expired `running` leases. That regressed the operator retry the old thread-based path gave for free. `enqueue_enrichment_job()` now returns an explicit outcome - `created`, `requeued`, `active` or `completed` - and atomically resets a `failed` job to `pending` with a fresh retry budget. It keeps the same job row, its last error message (audit) and its checkpoint (so the retry resumes), never revives a `completed` job, and never disturbs a live `running` lease. The conditional UPDATE is the whole guard, so concurrent re-POSTs converge on one logical job. Admission migration cannot leave an INVALID index ------------------------------------------------- `CREATE UNIQUE INDEX CONCURRENTLY uq_scans_one_active_per_subscription` fails, and leaves an unusable index behind, on a deployment that already holds several active scans for one subscription. The migration now preflights, changes nothing, and names the offending subscriptions in an actionable error; deciding which production scan is authoritative stays an operator call, and no scan history is deleted. A retry is safe: an INVALID index from an interrupted build is dropped before rebuilding. PostgreSQL tests no longer race the shared queue ------------------------------------------------ Fixtures called `claim_next_pending_scan()`, which takes the globally oldest pending scan, then persisted against the scan they had just created - so any unrelated pending row made `save_scan()` raise LostLease. Reproduced on a real database: with one older pending scan present, the old fixture claims someone else's row and fails; the new one does not. `claim_next_pending_scan()` and `claim_next_enrichment_job()` take an optional `scan_id` so a caller can claim a known row under identical lease and fencing semantics, and the fixtures use it. Queue-wide `recover_stale_*()` counts are asserted as progression of the test's own row rather than as global totals. Follow-ups ---------- - worker_heartbeats is bounded: rows past WORKER_HEARTBEAT_RETENTION_SECONDS are pruned on the beat that registers a new worker identity - once per process, not on every beat, and never for a live worker. - The scan and enrichment queues alternate one item per loop iteration instead of draining enrichment first, so neither can starve the other. - Added the partial index that keeps /metrics' last-successful-scan lookup from degrading into a sequential scan as history grows. - Documented SCAN_LEASE_SECONDS, SCAN_HEARTBEAT_SECONDS, OPENSHIELD_MAX_SCANS_PER_SUBSCRIPTION_PER_HOUR and WORKER_HEARTBEAT_RETENTION_SECONDS in .env.example. - docs/api-reference.md now documents the real admission and enrichment contracts, including which status codes are actually returned. Tests ----- New PostgreSQL coverage for terminal-failure requeue (explicit requeue, no restart of a completed job, a live lease is not stolen, concurrent requeues converge, a stale token cannot write after reclaim, a requeued job completes), the duplicate-active-scan migration preflight, stale heartbeat pruning, and worker fairness with both queues backlogged. Full suite on postgres:16-alpine: 897 passed, 2 skipped. The one failure, test_vector_store_purity, is a local checkout artifact - it needs a BM25 index this checkout's ai/vectorstore lacks, and passes in a clean worktree. Signed-off-by: Shaurya K Sharma <shauryaksharma24@gmail.com>
scan() only ever reports violations, so a scan with no findings for a rule is indistinguishable from compliant, not-applicable, and never evaluated. get_compliance_score() inferred PASS from the absence of a finding, silently treating errored or unmigrated rules as passing. Rules can now additionally expose evaluate(azure_client, subscription_id) -> List[RuleEvaluation], reporting a PASS/FAIL/ UNKNOWN/ERROR/NOT_APPLICABLE status per resource instead of only per violation. This is additive: scan() is unchanged, and a rule without evaluate() still runs, its coverage recorded as UNKNOWN/ LEGACY_RULE_NOT_MIGRATED rather than assumed to be a pass. - New rule_evaluations table (migration chained after OWASP#308's head), with CHECK constraints on the five statuses, a non-empty canonical resource_id, and a required reason_code for UNKNOWN/ERROR/ NOT_APPLICABLE. FAIL evaluations get a nullable finding_id FK, backfilled in the same transaction as the finding insert. - Engine wiring: evaluator exceptions produce an ERROR at a canonical rule/subscription scope rather than vanishing; a FAIL evaluation contributes its own finding only when scan() hasn't already reported the same (rule_id, resource_id), so a rule implementing both never double-counts. - get_compliance_score() now derives PASS/FAIL/UNKNOWN/ERROR/ NOT_APPLICABLE from rule_evaluations instead of inferring PASS from missing findings, and returns evaluated/passed/failed/unknown/error/ not_applicable counts separately. UNKNOWN and ERROR never improve the score. - AZ-KV-006 migrated as the reference evaluate() implementation. Single Alembic head and migration-chain checks, plus populated-database CHECK constraint tests gated on DATABASE_URL (CI already runs these against a live migrated Postgres). Signed-off-by: Dipesh Ray <dipesh.ray.g@gmail.com>
- Join the two-line CHECK constraint SQL string in the rule_evaluations migration onto one line, under the 120-char limit. - Add the blank line ruff format wants before a top-level def in the evaluate() code sample, and drop an em dash from the surrounding text. Signed-off-by: Dipesh Ray <dipesh.ray.g@gmail.com>
…sert Per the OWASP#321/OWASP#325 (OWASP#303) reconciliation discussion: a retried or replayed scan result must converge on the same rule_evaluations rows via ON CONFLICT (scan_id, rule_id, resource_id) DO UPDATE, not a delete-then-reinsert that could momentarily leave a concurrent reader seeing zero coverage for a scan that already has some. Rows for a rule/resource no longer present in the current evaluation set are removed afterward (delete-absent), scoped to the current scan. Findings persistence is unchanged (still delete-then-reinsert): OWASP#325 owns the finding_key/upsert model for findings, since that requires schema OWASP#321 doesn't have. This change is scoped to rule_evaluations, which is OWASP#321's own table. Signed-off-by: Dipesh Ray <dipesh.ray.g@gmail.com>
4754c1a to
101d47a
Compare
|
Pushed the save_scan change for rule_evaluations: it now upserts via Findings persistence is unchanged here, still delete-then-reinsert, since the finding_key/upsert model for findings needs the schema #325 owns. This change is scoped to rule_evaluations, which is this PR's own table. 960/960 tests pass, ruff clean, single Alembic head confirmed ( |
|
Reviewing the updated commits (3972519, 6f18d6b, 101d47a). The upsert persistence model works correctly end to end against a migrated Postgres (idempotent replay, FAIL linked to finding_id in the same transaction, delete-absent clears stale rows), and the migration, engine wiring, and compliance fix all check out. Full suite passes and CI is green. Three non-blocking notes, none of them holding up merge:
|
ritiksah141
left a comment
ritiksah141
left a comment
There was a problem hiding this comment.
Approving. Reviewed the updated commits (3972519, 6f18d6b, 101d47a) end to end.
Verified against a live migrated Postgres, not just mocks: the migration applies cleanly with a single Alembic head and all three CHECK constraints reject bad rows; save_scan upserts rule_evaluations idempotently (a replay converges instead of duplicating rows), links FAIL evaluations to their finding_id in the same transaction, and delete-absent removes rows no longer present; the engine records UNKNOWN for legacy rules and ERROR for evaluator exceptions, and dedupes FAIL findings against scan(); get_compliance_score reports UNKNOWN instead of a silent PASS and excludes NOT_APPLICABLE from the denominator. Full suite passes, ruff clean, CI green including DCO.
This matches the agreed reconciliation: this PR owns the evaluation contract and the evaluations upsert, while findings keep the delete-then-reinsert model because the finding_key upsert belongs to #325. Non-blocking nits are in a separate comment.
Confirming the merge order: this PR (#321) merges first as the canonical evaluation contract, then #325 rebases on top of 3f59f83a5253 and drops its duplicate rule_evaluations copy. Flagging for @m-khan-97 to sign off on the ordering so this can land.
TFT444
left a comment
TFT444
left a comment
There was a problem hiding this comment.
Reviewed the latest commits (3972519, 6f18d6b, 101d47a). CI is 20/20 green.
The two concerns I flagged both resolved on closer inspection. The NOT IN (unnest(...)) delete-absent is safe because RuleEvaluation.post_init guarantees non-null rule_id and resource_id. The SeverityContractError path aborts the entire scan anyway, so the missing ERROR row is moot.
Agreeing with the merge order: #321 first, then #325 rebases onto 3f59f83a5253.
Approving.
…ing pack The squashed dev rebase left four CI checks red: * Lint (ruff): get_latest_completed_scan() called cur2.execute on a cursor bound as cur (F821); one comprehension exceeded 120 cols. * Backend Tests / Container Scan: alembic revision graph had two heads — 3a76ff935bf6 and OWASP#321's 3f59f83a5253 both chained off d8e4f6a1b2c3. The container's startup `alembic upgrade head` failed the same way, so the Trivy job's runtime smoke test never came up. Rechained 3a76ff935bf6 onto 3f59f83a5253 (single head again). * Backend Tests (unmasked once alembic passed): get_score() referenced a non-existent self.subscription_id; get_compliance_score() re-derived the scan via an unscoped subquery, fetched rule_evaluations off the wrong cursor, and returned UNKNOWN where its own OWASP#302 test suite and evaluation_basis text call for findings-based PASS/FAIL. get_score() now scopes by subscription_id only when the attribute is set; compliance scoring is findings-based (no-finding + not engine-failed -> PASS, finding -> FAIL, failed_rule_ids -> NOT_EVALUATED), and NOT_EVALUATED now counts toward excluded_controls. Updated the one dev-era test that encoded the pre-OWASP#302 "no evaluation rows -> UNKNOWN" behaviour. * Rule & Compliance Validation: 20 controls newly on dev (AZ-IDN-016..025, AZ-STOR-006..009, AZ-DB-005..007, AZ-COSMOS-001/002, AZ-CACHE-001) plus the four mapping_pack_* header fields were absent the evidence schema in cis/iso27001/nist_csf/soc2. Filled in following the existing entries' conventions: N/A-* control IDs -> not_applicable; real framework IDs -> supporting/automated_configuration_scan; every entry review_status pending_review for human sign-off. Local: ruff clean, alembic heads == 1, validate_mapping_pack.py passes, full pytest 1035 passed (2 failures are pre-existing and need the CI Postgres service). Signed-off-by: parthrohit22 <parthrohit60@gmail.com>
…ing pack The squashed dev rebase left four CI checks red: * Lint (ruff): get_latest_completed_scan() called cur2.execute on a cursor bound as cur (F821); one comprehension exceeded 120 cols. * Backend Tests / Container Scan: alembic revision graph had two heads — 3a76ff935bf6 and OWASP#321's 3f59f83a5253 both chained off d8e4f6a1b2c3. The container's startup `alembic upgrade head` failed the same way, so the Trivy job's runtime smoke test never came up. Rechained 3a76ff935bf6 onto 3f59f83a5253 (single head again). * Backend Tests (unmasked once alembic passed): get_score() referenced a non-existent self.subscription_id; get_compliance_score() re-derived the scan via an unscoped subquery, fetched rule_evaluations off the wrong cursor, and returned UNKNOWN where its own OWASP#302 test suite and evaluation_basis text call for findings-based PASS/FAIL. get_score() now scopes by subscription_id only when the attribute is set; compliance scoring is findings-based (no-finding + not engine-failed -> PASS, finding -> FAIL, failed_rule_ids -> NOT_EVALUATED), and NOT_EVALUATED now counts toward excluded_controls. Updated the one dev-era test that encoded the pre-OWASP#302 "no evaluation rows -> UNKNOWN" behaviour. * Rule & Compliance Validation: 20 controls newly on dev (AZ-IDN-016..025, AZ-STOR-006..009, AZ-DB-005..007, AZ-COSMOS-001/002, AZ-CACHE-001) plus the four mapping_pack_* header fields were absent the evidence schema in cis/iso27001/nist_csf/soc2. Filled in following the existing entries' conventions: N/A-* control IDs -> not_applicable; real framework IDs -> supporting/automated_configuration_scan; every entry review_status pending_review for human sign-off. Local: ruff clean, alembic heads == 1, validate_mapping_pack.py passes, full pytest 1035 passed (2 failures are pre-existing and need the CI Postgres service). Signed-off-by: parthrohit22 <parthrohit60@gmail.com>
…WASP#263/OWASP#321) m-khan-97's review blocker: the previous head derived per-control status from findings + _scan_rule_outcomes only, so a rule with a persisted UNKNOWN/ERROR evaluation row (or a legacy rule with none) was reported PASS whenever it produced no finding, even though the response still declared contract_version 2. It also excluded NOT_EVALUATED from the denominator, which could raise the reported score as evidence was lost. get_compliance_score() now: - rolls up each rule's rule_evaluations rows for the scan via aggregate_status() (FAIL > ERROR > UNKNOWN > PASS > NOT_APPLICABLE); - reports UNKNOWN, never PASS, for an in-scope control whose rule has no evaluation row for the scan; - keeps _scan_rule_outcomes.failed_rule_ids only as a one-way stricter signal: a rule the engine recorded as failing to complete is forced to ERROR (never used to loosen a status or shrink the denominator); - excludes only mapping_type not_applicable/organizational from the score_percent denominator; UNKNOWN and ERROR stay in it and never count as a pass, so lost/missing evidence lowers the score; - keeps findings for severity/category/affected-resource detail only. Subscription scoping, NO_SCAN_DATA, NO_IN_SCOPE_CONTROLS, the mapping-pack snapshot/provenance/content-hash layer, and the per-control evidence-schema fields are all unchanged. evaluation_basis rewritten to describe the evaluation-derived semantics; api-reference.md and compliance-mapping-pack.md updated to match. Restores test_get_compliance_score_no_evaluation_rows_is_unknown_not_pass and adds persisted-UNKNOWN/ERROR, mixed PASS/UNKNOWN, worst-resource-wins, and ERROR-stays-in-denominator coverage in test_compliance_scoring.py. Signed-off-by: parthrohit22 <parthrohit60@gmail.com>
Resolves the two must-fix items and the follow-ups raised in review of OWASP#325. Integration: leave the OWASP#263 evaluation contract to OWASP#321 ------------------------------------------------------ This branch created a second `rule_evaluations` table, near-identical to the one PR OWASP#321 adds, that no production code ever populated: `run_scan()` emits no `evaluations` key, so the storage, the upsert and its tests described a contract that could not be observed in production. Two `CREATE TABLE rule_evaluations` statements would also have broken whichever of OWASP#321/OWASP#325 merged second, independently of the Alembic head ordering. Issue OWASP#263 owns that contract - the table, the PASS/FAIL/UNKNOWN/ERROR/ NOT_APPLICABLE semantics, engine emission and the compliance-score fix - so this branch drops it entirely and keeps only what OWASP#303 asks for: the stable `finding_key` identity and its unique index. `save_scan()` marks where OWASP#321's evaluation writes belong, inside the fenced completion transaction, so they inherit the lease/ownership check without re-implementing it. Terminally failed enrichment jobs are recoverable again ------------------------------------------------------- After three failed attempts a job became `failed` and nothing could move it: enqueue used ON CONFLICT DO NOTHING, claim only selected `pending`, and stale recovery only handled expired `running` leases. That regressed the operator retry the old thread-based path gave for free. `enqueue_enrichment_job()` now returns an explicit outcome - `created`, `requeued`, `active` or `completed` - and atomically resets a `failed` job to `pending` with a fresh retry budget. It keeps the same job row, its last error message (audit) and its checkpoint (so the retry resumes), never revives a `completed` job, and never disturbs a live `running` lease. The conditional UPDATE is the whole guard, so concurrent re-POSTs converge on one logical job. Admission migration cannot leave an INVALID index ------------------------------------------------- `CREATE UNIQUE INDEX CONCURRENTLY uq_scans_one_active_per_subscription` fails, and leaves an unusable index behind, on a deployment that already holds several active scans for one subscription. The migration now preflights, changes nothing, and names the offending subscriptions in an actionable error; deciding which production scan is authoritative stays an operator call, and no scan history is deleted. A retry is safe: an INVALID index from an interrupted build is dropped before rebuilding. PostgreSQL tests no longer race the shared queue ------------------------------------------------ Fixtures called `claim_next_pending_scan()`, which takes the globally oldest pending scan, then persisted against the scan they had just created - so any unrelated pending row made `save_scan()` raise LostLease. Reproduced on a real database: with one older pending scan present, the old fixture claims someone else's row and fails; the new one does not. `claim_next_pending_scan()` and `claim_next_enrichment_job()` take an optional `scan_id` so a caller can claim a known row under identical lease and fencing semantics, and the fixtures use it. Queue-wide `recover_stale_*()` counts are asserted as progression of the test's own row rather than as global totals. Follow-ups ---------- - worker_heartbeats is bounded: rows past WORKER_HEARTBEAT_RETENTION_SECONDS are pruned on the beat that registers a new worker identity - once per process, not on every beat, and never for a live worker. - The scan and enrichment queues alternate one item per loop iteration instead of draining enrichment first, so neither can starve the other. - Added the partial index that keeps /metrics' last-successful-scan lookup from degrading into a sequential scan as history grows. - Documented SCAN_LEASE_SECONDS, SCAN_HEARTBEAT_SECONDS, OPENSHIELD_MAX_SCANS_PER_SUBSCRIPTION_PER_HOUR and WORKER_HEARTBEAT_RETENTION_SECONDS in .env.example. - docs/api-reference.md now documents the real admission and enrichment contracts, including which status codes are actually returned. Tests ----- New PostgreSQL coverage for terminal-failure requeue (explicit requeue, no restart of a completed job, a live lease is not stolen, concurrent requeues converge, a stale token cannot write after reclaim, a requeued job completes), the duplicate-active-scan migration preflight, stale heartbeat pruning, and worker fairness with both queues backlogged. Full suite on postgres:16-alpine: 897 passed, 2 skipped. The one failure, test_vector_store_purity, is a local checkout artifact - it needs a BM25 index this checkout's ai/vectorstore lacks, and passes in a clean worktree. Signed-off-by: Shaurya K Sharma <shauryaksharma24@gmail.com>
PR OWASP#321 landed the rule_evaluations coverage contract on dev while this branch was outstanding. Both branches added their first migration on top of d8e4f6a1b2c3, which forked the Alembic chain into two heads. Repoint the first lease migration at 3f59f83a5253 so the chain stays linear and `alembic heads` reports the single head d4a8c1e6b2f9. save_scan now performs OWASP#321's evaluation upsert and stale-coverage cleanup inside this change's fenced transaction rather than alongside it. The evaluation semantics are OWASP#321's, unchanged; what this adds is that a worker which lost its lease can no longer rewrite another owner's coverage rows. The finding upsert keeps RETURNING id so a replayed delivery reuses the existing finding row and FAIL evaluations stay linked to it instead of pointing at a row that was deleted and recreated. Signed-off-by: Shaurya K Sharma <shauryaksharma24@gmail.com>
Adapt OWASP#321's save_scan tests to the fenced signature: the ownership probe is answered by the mocked cursor so finding ids still start at 1 and the FAIL-to-finding linkage assertions keep their original meaning. Add PostgreSQL regression coverage for the integration itself: an owner writes both findings and coverage rows, a worker whose lease was reclaimed raises LostLease and leaves no coverage behind, and a replayed delivery converges on the same rows with the FAIL row still pointing at the same finding id rather than a renumbered one. rule_evaluations references scans, so the lease fixture now purges it before deleting the scan row. Signed-off-by: Shaurya K Sharma <shauryaksharma24@gmail.com>
4 participants
What does this PR do?
Adds the rule evaluation coverage contract from #263: rules can now report a status for every resource they looked at (PASS included), so a scan that never ran, errored, or came from an unmigrated rule can no longer be silently read as a pass.
Type of change
Testing
alembic headsresolves to exactly one head)Related issue
Addresses #263
What changed
scanner/evaluation.py:EvaluationStatus(PASS/FAIL/UNKNOWN/ERROR/NOT_APPLICABLE),RuleEvaluationdataclass,subscription_scope_id()for canonical non-empty scope identifiers, andaggregate_status()implementing the conservative FAIL > ERROR > UNKNOWN > PASS > NOT_APPLICABLE roll-up order.alembic/versions/3f59f83a5253_rule_evaluations.py, chained onto fix(core): enforce severity contract v1 #308's head (d8e4f6a1b2c3).rule_evaluationstable with CHECK constraints on the five statuses, a non-emptyresource_id, and a requiredreason_codefor UNKNOWN/ERROR/NOT_APPLICABLE.finding_idis a nullable FK, populated only for FAIL evaluations.scanner/engine.py: callsrule.evaluate()when a rule exposes it. A rule withoutevaluate()gets one UNKNOWN/LEGACY_RULE_NOT_MIGRATED row instead of silently having no coverage. Anevaluate()exception (or a non-list return) produces one ERROR at the canonical rule/subscription scope. A FAIL evaluation contributes its own finding only whenscan()hasn't already reported the same(rule_id, resource_id), so a rule implementing both never double-counts.api/models/finding.py:save_scan()persistsrule_evaluationsin the same transaction as findings and backfillsfinding_idfor FAIL rows via a join on(scan_id, rule_id, resource_id).get_compliance_score()now derives each control's status fromrule_evaluations(never from finding absence), returnsevaluated/passed/failed/unknown/error/not_applicablecounts separately, and excludes UNKNOWN/ERROR from the numerator so they can never improve the score. Response carriescontract_version: "2".scanner/rules/az_kv_006.py: migrated as the one reference rule.evaluate()reports PASS/FAIL per vault, UNKNOWN for a vault missing its properties payload (previously silently skipped byscan()), and NOT_APPLICABLE when the vault list is empty (sinceAzureClient.get_key_vaults()can't distinguish "no vaults" from "the list call failed"; closing that gap is the deferred ARG cross-check).docs/adding-a-rule.md: short opt-in section documenting theevaluate()contract for future rule migrations.Regression coverage
evaluate()) is recorded as UNKNOWN/LEGACY_RULE_NOT_MIGRATED, never PASS.evaluate()exception produces ERROR at canonical scope instead of vanishing.evaluate()returning a non-list is treated as an error, not silently accepted.scan()didn't already report it.scan()already reported.rule_evaluationsrows persist in the same transaction as findings, withfinding_idcorrectly linked (FAIL) or left null (UNKNOWN/ERROR/PASS/NOT_APPLICABLE).rule_evaluationsrows atomically, same as findings.aggregate_status()conservative ordering (FAIL beats ERROR beats UNKNOWN beats PASS beats NOT_APPLICABLE) under every input order.get_compliance_score(): a clean scan with real PASS evaluation rows shows PASS; a clean scan with zero evaluation rows shows UNKNOWN, not PASS (the bug this PR fixes); a remediated rule with a new PASS row shows PASS; worst-severity aggregation across findings is unchanged.evaluate(): compliant vault -> PASS, non-compliant -> FAIL with the finding embedded, missing properties -> UNKNOWN, empty vault list -> NOT_APPLICABLE, mixed vaults report one status each.resource_id, reject a NULLreason_codeon UNKNOWN, and accept a valid PASS row withfinding_idleft null.d8e4f6a1b2c3.Scope
Matches what was agreed on #263: the contract, persistence/aggregation, and one end-to-end reference rule (AZ-KV-006). The Azure Resource Graph completeness cross-check stays a separate follow-up, as discussed.
Checklist
Signed-off-bytrailer