fix(core): enforce severity contract v1

[m-khan-97]

Why

We have two CRITICAL scanner rules today, but the old scoring paths treated CRITICAL as a zero-point deduction. That meant an all-critical scan could still report a score of 100, while resources and prioritization could downgrade the same findings to NONE or LOW.

For an enterprise security product, that is a trust failure. I have treated this as a cross-product data contract rather than patching individual screens.

What I changed

• Added a versioned severity contract covering IDs, aliases, rank, posture weight, matrix risk, labels, colours, and UI tones.
• Wired the scanner, persistence layer, API validation, AI ordering, resources, prioritization, compliance output, Sentinel ingestion, and frontend to that contract.
• Made unknown severities fail closed. INFORMATIONAL is accepted only as an input alias and is stored and returned as INFO.
• Added a migration that inventories existing values, normalizes supported legacy data, rejects unknown data, enforces the database constraint, repairs completed scan scores, and records score-contract provenance.
• Changed deployment order so the migration-owning API must become live before a worker for the same SHA is created.
• Added contract, scanner, persistence, API, monitoring, UI-wiring, and rollout-order regressions, plus contributor and architecture documentation.

Verification

• 128 focused backend and contract tests passed.
• Broader local suite: 721 passed, 3 skipped. Three dependency-only modules unavailable on this host were excluded; clean CI will install and exercise the complete dependency set.
• Ruff check and format, Python compilation, and diff checks passed.
• Node 22.22: severity contract tests 5/5, ESLint passed, and the production Vite build completed successfully.
• The frontend mirror is byte-identical to the canonical contract.

Rollout note

This migration intentionally corrects historical completed scores, so score changes are expected and auditable. Contract provenance stays nullable during rollout: if a legacy worker finishes after the schema change, its old score cannot be mislabeled as v1. The API now has to finish deployment before the new worker is created.

Before production, I want this migration run against a production-sized staging copy and the old workers drained. Downgrading the schema will not pretend the previously incorrect scores were valid.

Kept out of this PR

• Evidence completeness and UNKNOWN/NOT_APPLICABLE semantics remain in feat: persist PASS/FAIL/ERROR/NOT_APPLICABLE per rule per resource, fix compliance score #263.
• Transactionality, retry idempotency, finding ownership, and rollback recovery remain in core: harden scan transactions, leases, idempotency, and durable background work #303.

Closes #295
Related to #263 and #303

[github-actions]

Dependency Review

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

Scanned Files

None

[ritiksah141]

All good.

[TFT444]

Migration d8e4f6a1b2c3 has the same down_revision = 'c7a2e9f1b3d4' as PR #310's migration. These two PRs cannot both merge without Alembic branching. Coordinate with the #310 author so one migration correctly chains off the other before either is merged.

[m-khan-97]

@TFT444 @parthrohit22, thanks for catching the Alembic collision. I have rebased #308 onto the latest dev and kept d8e4f6a1b2c3 directly after the current dev head c7a2e9f1b3d4. The intended order is #308 first because #293 already depends on the severity contract; after #308 merges, #310 should rebase onto dev and chain its migration from d8e4f6a1b2c3 rather than creating a sibling head.\n\nI also tightened scan persistence during the rebase: retries now replace findings atomically, child findings are forced onto the parent scan ID, and SQL failures roll back before the worker records failure. Local verification: 42 focused tests passed; broader backend run passed 747 tests with 5 expected skips. Fresh CI is running on a194a71. Please re-review once it is green.

[parthrohit22]

Migration is genuinely well-built: validate (fail loud via RAISE EXCEPTION on any out-of-set severity) -> normalize -> add column -> add CHECK NOT VALID then VALIDATE CONSTRAINT separately (correct low-lock ordering) -> repair scores only once the constraint guarantees every severity is canonical. Hand-verified the score arithmetic (GREATEST(0, 100 - COALESCE(SUM(deduction),0)) with CRITICAL=20/HIGH=10/MEDIUM=5/LOW=2/INFO=0) against a few example rows and it matches openshield/severity.py exactly, and the contract/JSON/frontend-mirror sync is byte-exact and CI-enforced (sync-severity-contract.mjs --check). scanner/engine.py re-raising SeverityContractError out of the per-rule try/except (rather than silently dropping just that finding) and worker.py marking the scan failed rather than completed is exactly the fail-loud behavior this PR is going for, and it's correct.

One real gap though, left as an inline comment: sentinel/ingest.py still silently defaults a missing severity key to MEDIUM in a function this PR otherwise rewrote to fail loud on bad severities. That's the exact anti-pattern the PR description cites as the whole reason for this change, in a file the PR already touched, so I don't think it should ship as a known gap.

Three more non-blocking notes, no action needed to merge:

• test_v1_migration_freezes_the_same_ids_and_weights_as_the_contract only does static text-matching against the migration source (asserts a string like "WHEN 'CRITICAL' THEN 20" appears); the actual data-repair SQL only ever runs against an empty DB in CI. Worth a real integration test that seeds a couple of completed scans with mixed severities and asserts the resulting scans.score, since that's the riskiest, most novel part of this migration.
• That same test couples a frozen historical migration file to the live severity.py contract module - if a future v2 contract changes a weight, this test would fail unless someone edits an already-applied migration, which docs/severity-contract.md's own guidance says not to do. Worth flagging for whoever writes v2.
• This PR is what introduces synchronous throwing on unrecognized severity into more frontend components (SeverityBadge/RiskBadge/severityDefinition), and there's no ErrorBoundary anywhere in the frontend - an unexpected value reaching one of these would blank the whole page rather than degrade. Not a regression from correct behavior, just worth a follow-up ticket.

Separately: gh pr view shows this PR conflicting against dev right now (unrelated to the code review) - flagging since #310 also touches scans via a migration and the two would fork the Alembic revision graph if merged independently; I've noted the coordination needed on that side.

Full suite (Python 3.11, this sandbox's .venv targets 3.14 which can't build psycopg2-binary): 732 passed, 3 skipped, 2 failed - the 2 failures are test_observability.py hitting an unreachable postgres host in this environment, unrelated to this diff. All severity-contract-relevant tests (severity_contract, sentinel_input_validation, prioritization, resources, clean_scan, engine_integration, input_validation, error_exposure, render_deploy_config) pass 99/99, and frontend npm run test:severity passes 5/5 including the contract sync check.

[parthrohit22]

This still defaults a missing severity key to "MEDIUM" before validation runs. The surrounding logic in this same function was rewritten by this PR to raise on an unrecognized severity value, but a missing key silently becomes MEDIUM rather than failing loud - which is exactly the "an all-critical scan could still report a passing score" trust problem this PR's own description calls out as the reason for the whole change.

Failure scenario: a hand-edited or corrupted findings payload fed to sentinel/ingest.py is missing severity on a record that was actually CRITICAL. Instead of the ingest rejecting the record, it ships a fabricated MEDIUM to the customer's Sentinel workspace. tests/test_sentinel_input_validation.py only exercises explicit invalid/valid severity strings, never a missing key, so this is currently unguarded.

api/validation.py's findings_list handles the same situation by leaving severity as None rather than coercing it - suggest matching that here (raise/reject on missing, don't default).

[m-khan-97]

@parthrohit22, thanks for catching the Sentinel gap. It is fixed in 5937dab: missing, null, and empty severities are now rejected instead of defaulting to MEDIUM, with regression coverage for all three cases. I also rebased onto the latest dev and preserved both #287's dashboard load/error states and the severity-contract behavior during conflict resolution.\n\nVerification: 102 severity-focused tests passed; the broader backend suite passed 750 tests with 5 expected skips. Fresh CI is running. Please re-review once it is green.

[parthrohit22]

The gap I flagged is genuinely closed. Pulled this branch's actual head into a clean clone and verified rather than trusting the summary: sentinel/ingest.py now raises ValidationError("severity is required for every Sentinel finding") when the key is missing, None, or empty string, before it ever reaches _safe_text/normalize_severity - the exact "an all-critical scan could still report a passing score" anti-pattern this PR set out to eliminate no longer has a gap in the one file it left open last round. test_normalise_rejects_missing_or_empty_severity is parametrized on all three real cases ({}, {"severity": None}, {"severity": ""}) and I ran it myself: passes, along with the rest of test_sentinel_input_validation.py and all 22 tests in test_severity_contract.py. Full suite: 735 passed, 5 skipped, 0 failed. Ruff clean. Migration chain resolves cleanly (alembic heads -> single head at d8e4f6a1b2c3).

None of my three non-blocking notes from last round needed to hold this up and I'm not asking for them now - just flagging I didn't see them addressed in case they got lost: the migration-integration-test gap (data-repair SQL still only runs against an empty DB in CI), the frozen-migration/live-contract coupling in test_v1_migration_freezes_the_same_ids_and_weights_as_the_contract, and the missing frontend ErrorBoundary now that more components throw synchronously on bad severity.

Migration coordination with #310 is already tracked on both sides (you kept d8e4f6a1b2c3 chained after c7a2e9f1b3d4 here, and I've got #310 documented to rebase onto whichever of us merges second) - not a new blocker, just confirming it's still the right call now that I can see both PRs' current state.

Approving.

[parthrohit22]

Re-reviewed from scratch on the current head - the code hasn't changed since my last approval (5937dab is both the commit I approved and today's head), but you re-requested review and I'm not going to just rubber-stamp my own old finding without checking, so I re-verified everything myself rather than trusting memory.

What I re-ran and re-checked just now:

• All 70 of this PR's own new/changed tests (test_severity_contract.py, test_sentinel_input_validation.py, test_engine_integration.py, test_clean_scan.py, test_prioritization.py, test_resources.py, test_input_validation.py) - all pass.
• Full backend suite: 726 passed. (One unrelated failure locally - test_chroma_default_embedding_uses_onnx_runtime - is this branch's own stale copy of tests/test_rag_dependencies.py from before #317 removed chromadb; not a defect in this PR's own diff, just a symptom of the branch being unrebased since before #316/#309/#317 landed on dev. See the rebase note below.)
• frontend/src/utils/severity.test.mjs: 5/5 pass.
• Read the actual sentinel/ingest.py fix directly (not just the test result) to confirm my original finding is really closed: missing/empty severity is rejected before normalization is even attempted (line 75-76), and anything outside the contract is rejected via normalize_severity() + SeverityContractError (line 78-81). Matches the test coverage exactly - confirmed, not just claimed.
• Read the full migration (d8e4f6a1b2c3_severity_contract_v1.py) line by line. This is genuinely careful work: a pre-flight DO $$ ... RAISE EXCEPTION block that refuses to proceed if any existing severity value can't be mapped (fails loud instead of silently corrupting data), the ADD CONSTRAINT ... NOT VALID + VALIDATE CONSTRAINT pattern (avoids a long-lived table lock on a large findings table - the right way to add a CHECK constraint in production Postgres, not the naive one-shot ALTER TABLE ADD CONSTRAINT), and a rescore of historical completed scans using the exact same weights as contracts/severity.v1.json (verified: 20/10/5/2/0 match exactly). The downgrade explicitly documents that corrected historical scores are not reverted, rather than leaving that ambiguous.

The one thing still open, and it's not something this PR can fix alone:

TFT444's CHANGES_REQUESTED is still active and still correct: d8e4f6a1b2c3 and PR #310's migration both chain off c7a2e9f1b3d4, and merging both as-is forks the Alembic revision graph. I can't approve that away, and neither PR can unilaterally resolve it - I tried pointing my own PR's migration at an unmerged sibling's revision file and it breaks that PR's own CI (KeyError on a revision that doesn't exist outside the sibling's branch), so the fix genuinely requires a merge-order decision: whichever of #308/#310 lands second rebases onto the new dev head and repoints down_revision. That's a maintainer call on ordering, not a code change here. I've already added a CI check on my side (#310) that asserts alembic heads returns exactly one head, so if this does get missed at merge time, it fails loudly instead of silently forking.

Also worth flagging since I checked it directly: this branch's merge-base with dev predates #316, #309, and #317 - it's meaningfully behind. Doesn't block my approval (the code itself is correct and tested), but it should be rebased before/at merge so CI reflects current dev rather than a stale pip-audit/test snapshot.

Approving - the actual code is solid and my original findings are genuinely fixed. The Alembic coordination is real but is a merge-order decision for a maintainer, not a defect in this PR.

[parthrohit22]

Confirmed this is the actual fix for my original finding, not just claimed - read it directly: missing/null/empty severity is rejected right here, before normalize_severity() is even called, so it can't fall through to a silent default. Matches test_normalise_rejects_missing_or_empty_severity's three parametrized cases, which I ran myself.

[parthrohit22]

This is where the fork with PR #310's migration lives - both chain off c7a2e9f1b3d4. Not something fixable from this side alone (confirmed by trying the equivalent on my own PR's migration - pointing down_revision at an unmerged sibling's revision file breaks that PR's own CI outright). This needs a merge-order call: whichever of #308/#310 merges second rebases onto the new head and repoints this. Not blocking my approval - flagging for whoever merges.

[Vishnu2707]

Approved. LGTM!

[Vishnu2707]

@TFT444 - Please approve and merge if everything looks good from your side.