feat(automation): finding lifecycle engine, scan outcome contracts, and pattern detection [1/5]

[TFT444]

Summary

Implements the finding lifecycle foundation described in issue #311 (Automation PR 1/5).

• 6 new DB tables via Alembic migration e1f2a3b4c5d6: scan_rule_outcomes, scan_lifecycle_applications, finding_fingerprints, finding_lifecycles, finding_lifecycle_transitions, patterns
• LifecycleService.apply_scan(): idempotent, fail-closed, single transaction with FOR UPDATE row locking; a finding only resolves when its rule and collectors succeeded over the same authorised inventory boundary
• PatternService.detect_and_publish(): detects three pattern types (persistent_finding, cross_resource_recurrence, reopened_finding) with deterministic thresholds stored in the record
• Patterns API: GET /api/v1/patterns and GET /api/v1/patterns/<id> with tenant-scoped IDOR protection (subscription enforced in SQL, cross-subscription requests rejected 400/404)
• Engine outcome recording: run_scan() now records SUCCESS / EMPTY_SUCCESS / PERMISSION_DENIED / TIMEOUT / FAILED per rule; Azure HTTP 403 detected as PERMISSION_DENIED
• Worker wiring: LifecycleService and PatternService called immediately after db.save_scan(); lifecycle failures are non-fatal so the scan record is always preserved

Test plan

• tests/test_finding_lifecycle.py — 18 unit cases covering idempotency, fail-closed behaviour, state transitions, and audit trail
• tests/test_patterns.py — 12 cases covering all three pattern types, IDOR regression (no-subscription-400, cross-subscription-400, cross-subscription-IDOR-404)
• Run pytest tests/test_finding_lifecycle.py tests/test_patterns.py -v locally (no live Postgres required; all DB calls mocked)
• Confirm GET /api/v1/patterns returns 400 without a subscription scope
• Confirm Alembic migration applies cleanly: alembic upgrade head

Notes

This is PR 1/5 for the finding lifecycle automation epic. Subsequent PRs will add remediation-agent integration, scheduled re-scan triggers, SLA tracking, and the prod-gate controls described in #311.

[github-actions]

Dependency Review

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

OpenSSF Scorecard

Package	Version	Score	Details
pip/multidict	6.9.1	🟢 6.6	Details Check Score Reason Code-Review ⚠️ 0 Found 1/30 approved changesets -- score normalized to 0 Binary-Artifacts 🟢 10 no binaries found in the repo Maintained 🟢 10 30 commit(s) and 24 issue activity found in the last 90 days -- score normalized to 10 Dangerous-Workflow 🟢 10 no dangerous workflow patterns detected Token-Permissions ⚠️ 0 detected GitHub workflow tokens with excessive permissions Pinned-Dependencies ⚠️ 0 dependency not pinned by hash detected -- score normalized to 0 CII-Best-Practices ⚠️ 0 no effort to earn an OpenSSF best practices badge detected Fuzzing 🟢 10 project is fuzzed License 🟢 10 license file detected Security-Policy 🟢 10 security policy file detected Packaging 🟢 10 packaging workflow detected SAST 🟢 10 SAST tool is run on all commits Branch-Protection 🟢 3 branch protection is not maximal on development and all release branches Signed-Releases 🟢 8 5 out of the last 5 releases have a total of 5 signed artifacts.
pip/werkzeug	3.1.9	Unknown	Unknown

Scanned Files

• requirements.txt

[m-khan-97]

@TFT444, please pause this automation PR before further implementation or review. It currently introduces scan_rule_outcomes and worker lifecycle writes that overlap #321’s canonical #263 evaluation contract and #325’s fenced/idempotent persistence architecture. Calling lifecycle services after save_scan() as non-fatal work also needs to be reconciled with #325’s atomic authoritative-write and fencing guarantees. In addition, backend CI is currently failing. We will resume #326 only after #321/#325 establish the shared schema and transaction boundary; then this PR should rebase onto those foundations and contain lifecycle/pattern behavior only, without a third scan-outcome contract.

[parthrohit22]

Looked through this — the transaction/fail-closed structure in LifecycleService.apply_scan() is solid (idempotency sentinel inserted last, FOR UPDATE locking, resolution gated on resolving_rule_ids so a scan with no clean outcomes can't silently resolve anything), and the Patterns API's tenant/subscription scoping is done right (JWT is authoritative, query param can only narrow, both columns actually applied in the WHERE clause, not just validated).

Holding off on approve/changes since this is still draft and CI has one red test: test_finding_lifecycle.py::TestLifecycleStateTransitions::test_reopened_finding_absent_from_success_scan_is_resolved (assert False). The resolution SQL itself looks right at a glance (correctly includes REOPENED in the state filter), so this reads more like a mock call-count mismatch in the test than a logic bug, but worth running locally before this comes out of draft. Ping me for another look once it's green and ready.

[github-actions]

This PR has been inactive for 14 days. Please update or it will be closed in 7 days.

[parthrohit22]

Heads-up @TFT444: #325 merged into dev and this PR now conflicts in api/routes/scans.py and scanner/worker.py, which #325 reworked for scan leases and fencing. When you resolve them, please keep the fenced save_scan path and the lease heartbeat intact, since the lifecycle hooks need to run inside that contract.

The migration also needs rebasing. e1f2a3b4c5d6 chains from d8e4f6a1b2c3, but dev's head is now d4a8c1e6b2f9. It also has the same revision ID as #352's graph migration, so one of the two needs a new ID.

Before your next review request, could you rebase onto current dev, check that alembic heads returns a single head, and rerun the Postgres-gated tests? Thanks!

[TFT444]

@ritiksah141 @parthrohit22 could you please re-review the latest head (9339a3c)?

Scanner syntax, verified tenant/subscription authorization, lifecycle resolution, migrations and CI fixes are updated.

The fixes are pushed and all checks are passing on this head (the deployment skip is expected). Please review the updated code and tests, and update your review decision or resolve the relevant conversations when satisfied.

[ritiksah141]

Re-reviewed at head 9339a3c. Verified locally: clean rebase onto dev tip ea5044f, single Alembic head with a8d9c2e4f601 correctly rechained onto b6d2f8a4c1e7, full upgrade/downgrade/upgrade cycle against real Postgres, the Postgres-gated lifecycle test passing against the migrated database, ruff check and format clean, and the full suite green apart from six TestComplianceRoute failures that reproduce identically on plain dev (local environment artifact, not this PR). All 21 CI checks pass on this head.

The core is solid. The double-gated resolution (rule execution in SUCCESS/EMPTY_SUCCESS plus an exact PASS evaluation set per rule and normalized resource, plus absence from current findings) is fail-closed in the right places, apply_scan is idempotent with the sentinel inserted last, and the patterns API scoping (token claims authoritative, query param can only match, cross-subscription detail returns 404 via SQL) is done properly. The legacy-rule behavior is correct too: rules without evaluate() produce UNKNOWN coverage, so their findings can never silently resolve.

Requesting changes on the following four items.

1. Lifecycle application must be durable, not best-effort.

The worker calls LifecycleService.apply_scan and PatternService.detect_and_publish in a plain try/except after save_scan has committed. If the process dies in that window, the scan is persisted but the lifecycle application is lost permanently: there is no job record, no retry, and no requeue. The next scan partially heals the state machine, but occurrence_count undercounts, resolution is deferred a full scan cycle, patterns for the missed scan are never published, and scan_rule_outcomes rows for that scan are silently absent. This is exactly the failure class that the scan durability work in #325 addressed for enrichment, and the mechanism is already on dev to copy: the enrichment job is created inside the fenced save_scan transaction (INSERT ... ON CONFLICT (scan_id) DO NOTHING) and processed with lease heartbeats, fencing, checkpoints, bounded attempts and exponential backoff requeue. Please apply the same pattern here: enqueue lifecycle application atomically with scan persistence, and process it with bounded retry. apply_scan is already idempotent, so retrying is safe. Acceptance criteria: a crash between the save_scan commit and the lifecycle commit must not lose the application; a failing apply must retry up to a bound and then park visibly; processing must be fenced. Implementation choice is yours: a sibling lifecycle_jobs table (a new migration or an extension of the unmerged a8d9c2e4f601) is cleaner than overloading enrichment_jobs, whose scan_id uniqueness constraint admits one job per scan. If you add a new revision, coordinate the chain with #352, whose graph migration currently points its down_revision at a8d9c2e4f601.

2. Tenant attribution is inconsistent between the write path and the read path.

The worker writes tenant_id from OPENSHIELD_TENANT_ID with no normalization, defaulting to subscription_id. The patterns API reads tenant_id from the verified token claim, lowercased in OIDC mode but not lowercased in shared-secret mode, and matches with case-sensitive SQL equality. Consequences: in a default deployment where OPENSHIELD_TENANT_ID is unset, every row is written with tenant_id equal to the subscription id while tokens carry the Entra tenant id, so the endpoint permanently returns empty results; if the env var is set with different casing than the token claim, results are silently empty as well; and nothing in the deployment documentation tells an operator what value to set (docs/security/authentication.md only says the variable cannot override reads). Please fix all three sides: normalize tenant_id identically on the write and read paths (lowercase both, in both auth modes), document OPENSHIELD_TENANT_ID in the deployment configuration docs with the exact expected value, and emit a startup or per-scan warning when it is unset so the failure mode is visible instead of silent.

3. Revert the POSTGRES_PORT expression change in ci.yml.

The change from job.services.postgres.ports[5432] to ports['5432'] is not fixing anything: the Container Scan job is green on current dev with the numeric form. The change also forces an edit to tests/test_container_runtime_config.py, which pins the expression deliberately. Please revert both the workflow line and the test edit to keep this diff scoped to the feature.

4. Remove the ephemeral stack branch names from the shared workflows.

The three feat/* branch names added to the pull_request triggers of ci.yml, codeql.yml, dco.yml, dependency-review.yml and website.yml, plus tests/test_feature_stack_ci.py which enforces them, should not merge into dev. Pull request runs use the workflow files from the base branch, and the stack branches already carry these triggers, so stacked PRs are covered without dev holding dead branch filters. Merging them leaves permanent cleanup debt plus a test that fails the moment someone removes the stale filters. If there is a concrete scenario where the triggers must live on dev, please describe it in the PR and we can revisit; otherwise drop the workflow edits and the test file from this PR.

Non-blocking, please address alongside the above or in the series:

• The PR body is stale: the migration is now a8d9c2e4f601 (e1f2a3b4c5d6 belongs to #352), the new tests number 24 plus 20 rather than 18 plus 12, and test_lifecycle_postgres.py now exists as a Postgres-gated test, so "no live Postgres required" no longer holds.
• _effective_tenant takes an effective_sub parameter it never uses.
• patterns.finding_ids stores lifecycle ids for cross_resource_recurrence; rename the column or document the meaning.
• The engine's completed_at timestamp is discarded when inserting scan_rule_outcomes (NOW() is used instead); persist the value you already produce.
• Findings on deleted resources can never resolve under the current design (no PASS evaluation exists for a resource that is gone, and there is no manual state-change endpoint yet). Fine for 1/5, but please track it in the series, along with a retention policy for the patterns table, which currently grows by one row per type, lifecycle and scan forever.
• The Werkzeug and multidict floors are a justified security bump and the lockfiles regenerate cleanly; no objection to keeping them here.

Once items 1 through 4 are addressed I am happy to re-review promptly.

[SHAURYAKSHARMA24]

ritiksah141's four items cover the durability, tenant casing and workflow changes, so I won't repeat those. Two more things came up when I ran this together with #381, plus one case to add to item 1.

Doesn't work once #381 lands. After #381, a rule with evaluate() goes through _run_evaluate() and hits continue before the scan() branch, so it never gets a rule_outcomes entry. apply_scan() only resolves when outcome_by_rule[rule] is in _RESOLVING_STATUSES, so for a migrated rule a fixed resource stays OPEN for good. This PR's own test shows it: merge #382 (which contains #381) into this branch and test_engine_retains_legacy_outcomes_and_resource_evaluations fails with an IndexError on rule_outcomes[0]. I also ran it end to end on Postgres with KV-006: violating scan -> OPEN, then a scan where the vault is fixed (PASS) -> still OPEN, with no outcome row. On this branch alone the same sequence resolves correctly. Since #380 moves every rule onto evaluate(), resolution would gradually stop working everywhere.

That and the EMPTY_SUCCESS comment inline point the same way as m-khan-97's note from August. I'd drop rule_outcomes and resolve from what the engine already produces: a (rule, resource) resolves when its evaluation in this scan is PASS and the rule isn't in failed_rule_ids, and ERROR/UNKNOWN leaves it alone. That behaves the same before and after #381, and it avoids a second outcome signal that can disagree with the evaluations.

Apply order (adds to item 1). Nothing in apply_scan() checks whether the scan being applied is older than one already applied. On Postgres: A (violating) applied -> OPEN, B (violating) saved but not applied yet, C (fixed) applied -> RESOLVED, then B applied -> REOPENED, even though C is the newest evidence. With the current inline call that needs two overlapping scans, but a retry queue makes it a normal path, so the lifecycle job should no-op when a newer scan for the subscription has already been applied.

Happy to share the scripts I used for these if useful.

[SHAURYAKSHARMA24]

For legacy rules this records EMPTY_SUCCESS when the scan couldn't see anything. The collectors catch the exception and return [] (e.g. get_storage_accounts, get_network_security_groups), so scan() returns normally and the 403 branch below never fires. I patched the SDK clients to raise a 403, a 429 and a ClientAuthenticationError; every rule wrote EMPTY_SUCCESS to scan_rule_outcomes. It doesn't resolve anything wrongly on its own because a PASS is also required, but it's a stored row saying the rule succeeded, and patterns/reporting will read it.

[SHAURYAKSHARMA24]

This is where the apply-order case bites: a late apply of an older scan moves RESOLVED back to REOPENED. It would do the same to ACCEPTED/SUPPRESSED, which are user decisions, so a stale apply could undo a suppression.

[SHAURYAKSHARMA24]

Adding to ritiksah141's tenant point: Entra access tokens don't carry a subscription_id claim, so in OIDC mode this raises for every real user and the patterns endpoints always return 400, even with tenant casing fixed. The allowed subscriptions probably need to come from server-side config or a principal-to-subscription mapping rather than the token.

[TFT444]

Hi @parthrohit22 @ritiksah141 @Vishnu2707 @SHAURYAKSHARMA24 @vogonPrayas - all review findings have been addressed and pushed (76a656c):

• exc_info=True added to lifecycle error log in worker.py so stack traces appear in production
• autocommit=False guard added at entry of apply_scan in lifecycle_service.py
• Dead effective_sub param removed from _effective_tenant in patterns.py
• OPENSHIELD_TENANT_ID documented in .env.example

All 91 tests pass locally. CI is fully green. Ready for re-review, would appreciate an approve if this looks good.