[TFT444]

Summary

Saving a scan now enqueues lifecycle work in the same transaction. Lifecycle transitions, pattern publication and job completion commit atomically under the job fence and tenant/subscription lock. Expired workers cannot write; delayed scans cannot rewind newer evidence or publish patterns under an older scan ID. Resolution uses resource PASS evidence with failed-rule safeguards. Internal reads process findings beyond the public 1,000-row cap, and cross-resource patterns count distinct resources.

Runtime dependency inputs and Linux Python 3.11 hash locks include the patched aiohttp, multidict, Werkzeug and certifi versions. No new vulnerability suppressions were added.

Validation

• Exact head: 87e870d0eb30df83323ec8b65c5cfe9aa36aa0e0.
• Completed CI run: green, including backend, lint, security scans, dependency audit, container scan and builds.
• Full backend suite: 1,710 passed, 3 skipped, 4 subtests passed; 88.62% coverage against the 80% gate.
• The three skips are optional AI index/LLM checks, not newly skipped regressions.

Pattern reads accept verified signed scope or a matching tenant deployment's authorized subscription configuration. Missing tenant configuration fails closed. PostgreSQL regressions cover rollback, retries, stale ownership, ordering, 1,001 findings, resource fingerprint variants and subscription selection. Explicit read-only workflow triggers cover this named feature stack; deployment triggers are unchanged.

Review status

Code and current-head CI are ready for independent re-review. Existing reviewer approvals and conversations are intentionally left for reviewers to confirm; this is not approval or merge authorization.

[github-actions]

Dependency Review

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

OpenSSF Scorecard

Package	Version	Score	Details
pip/aiohttp	3.14.5	Unknown	Unknown
pip/certifi	2026.10.23	🟢 6.5	Details Check Score Reason Code-Review 🟢 6 Found 4/6 approved changesets -- score normalized to 6 Security-Policy 🟢 10 security policy file detected Binary-Artifacts 🟢 10 no binaries found in the repo Maintained 🟢 8 8 commit(s) and 2 issue activity found in the last 90 days -- score normalized to 8 Token-Permissions 🟢 10 GitHub workflow tokens follow principle of least privilege Dangerous-Workflow 🟢 10 no dangerous workflow patterns detected Pinned-Dependencies 🟢 5 dependency not pinned by hash detected -- score normalized to 5 CII-Best-Practices ⚠️ 0 no effort to earn an OpenSSF best practices badge detected License 🟢 9 license file detected Fuzzing ⚠️ 0 project is not fuzzed Signed-Releases ⚠️ -1 no releases found Branch-Protection ⚠️ 0 branch protection not enabled on development/release branches Packaging 🟢 10 packaging workflow detected SAST ⚠️ 0 SAST tool is not run on all commits -- score normalized to 0
pip/mako	1.4.3	Unknown	Unknown
pip/multidict	7.2.1	Unknown	Unknown
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!

[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.

[parthrohit22]

Re-reviewed ef8f6e7. The autocommit guard and the dead-parameter cleanup are good. But none of @ritiksah141's four items from 9 Oct are addressed yet, and the new evaluate()-only path has a gap of its own.

1. Still open from the 9 Oct reviews

• Durability (Ritik 1): lifecycle and pattern application is still a try/except after save_scan commits. A crash in that window still loses the application permanently.
• Tenant casing (Ritik 2): scanner/worker.py:226 still writes OPENSHIELD_TENANT_ID unnormalised, falling back to the subscription ID, and _effective_tenant() still compares case-sensitively. In a default deployment the patterns endpoint still returns nothing.
• CI edits (Ritik 3 and 4): the ports['5432'] change, the feat/* triggers in five workflows and tests/test_feature_stack_ci.py are all still in the diff.
• Apply order (@SHAURYAKSHARMA24): nothing stops an older scan from being applied after a newer one, so the B-after-C reopen still happens. Once item 1 adds a retry queue, this becomes a normal path rather than a race.

2. evaluated_cleanly ignores failed_rule_ids

e33ff03 treats an evaluate()-only rule as having completed cleanly if any of its evaluations isn't ERROR. With #369, the engine keeps the valid items when evaluate() returns a partly malformed list, adds an ERROR evaluation and puts the rule in failed_rule_ids. Under this check that rule still counts as clean, so its PASS resources resolve, while compliance scoring treats the same rule as ERROR for the same scan. The two outputs disagree.

Shaurya's rule avoids this and doesn't need rule_outcomes at all: a (rule, resource) resolves when its evaluation in this scan is PASS and the rule is not in the scan's failed_rule_ids; ERROR/UNKNOWN leave it alone. Please pass failed_rule_ids into apply_scan() and gate on it, with a test where a rule has one PASS and one ERROR evaluation and the PASS resource is not resolved.

Merge order

#352, #353 and #354 now chain onto this PR's a8d9c2e4f601, and #384 chains off the same parent (b6d2f8a4c1e7). Whichever lands after this needs to repoint. The ritik non-blocking list (stale PR body, finding_ids naming, completed_at being dropped, deleted-resource resolution, patterns retention) still applies.

[TFT444]

All 4 blockers have been addressed. Changes pushed to this branch:

• CI workflow reverted to numeric port form; feat/* triggers removed from all 5 workflow files
• Tenant ID casing normalized end-to-end (worker write path + shared-secret auth read path)
• failed_rule_ids gate prevents PASS evaluations from resolving findings when a rule partially errors
• Durable lifecycle_jobs queue (fenced lease, bounded retry, migration) replaces the inline lifecycle call

Ready for re-review @ritiksah141 @parthrohit22 @Vishnu2707 @SHAURYAKSHARMA24

[parthrohit22]

Re-reviewed ecf6d71. Thanks, this is a big step forward. I checked the earlier blockers and they’re resolved:

Lifecycle jobs are queued in the same transaction as save_scan, with a fenced lease and limited retries.
There’s an advisory lock, and an older scan can no longer be applied after a newer one.
A rule in failed_rule_ids can no longer resolve findings.
Tenant IDs are lowercased on write and read.
The CI scaffolding is gone.

One blocker remains:

scanner/lifecycle_worker.py needs OPENSHIELD_TENANT_ID or AZURE_TENANT_ID. With managed identity often neither is set. Then every job fails three times, ends as failed, and the feature switches itself off with only a log line.
Please check this when the worker starts (fail fast or emit a clear warning/metric). Also document it as required in .env.example, which currently leaves it blank.

Also, please drop the requirements* changes, since OWASP/openshield#401 covers them. Once both are done I’m happy to approve. @ritiksah141 @SHAURYAKSHARMA24, a final pass from you on the lifecycle job path would be good.

[TFT444]

@parthrohit22 @ritiksah141 Requirements changes have been dropped since #401 covers them, and the OPENSHIELD_TENANT_ID startup check is already in place in scanner/lifecycle_worker.py. Would appreciate a final pass when you get a chance.

[TFT444]

@parthrohit22 Startup warning added in scanner/lifecycle_worker.py at module level so the worker logs immediately on boot if OPENSHIELD_TENANT_ID and AZURE_TENANT_ID are both unset. Requirements changes were also dropped. Ready for your final pass.

[TFT444]

@parthrohit22 last item addressed: scanner/lifecycle_worker.py now warns fail-closed at startup if OPENSHIELD_TENANT_ID/AZURE_TENANT_ID are unset (3b759fa), .env.example documents it, and requirements* reverted to dev baseline per #401 (f00436c). All 21 checks green. @ritiksah141 @SHAURYAKSHARMA24 a final pass on the lifecycle path would be appreciated.