Feat(ci): persist CI/CD scan results (#259 PR 2/3)

[emon22-ts]

Summary

PR 2/3 for #259: persist CI/CD (AZ-CI-001..004) scan results.

• alembic/versions/e1f2a3b4c5d6_ci_scans.py: new ci_scans and ci_scan_evaluations tables (chained off 3f59f83a5253)
• api/models/ci_scan.py: CIDatabase persistence layer (pending/running/completed/failed lifecycle, atomic save, read helpers for PR 3/3)
• scanner/ci_engine.py: CIScanEngine.run_and_persist() and a --persist CLI flag; run_scan() is unchanged
• tests/test_ci_persistence.py: 25 tests, all DB access mocked

Design notes

• CI scans get their own tables instead of reusing scans, because scans.subscription_id is NOT NULL and Azure-specific. ci_scan_evaluations mirrors the rule_evaluations constraints (status enum, non-empty resource_id, reason_code required for UNKNOWN/ERROR/NOT_APPLICABLE).
• create_pending_ci_scan is idempotent (ON CONFLICT DO NOTHING), so the PR 3/3 route can create the pending row for its 202 response and then call run_and_persist(scan_id=...).
• A retry of the same scan_id converges on the latest result (header upserted, evaluation rows replaced in one transaction).

Testing

• Full suite: 1135 passed, 9 skipped, 0 failed (Python 3.11).
• The migration has not been run against a real Postgres in my environment; please run alembic upgrade head and alembic downgrade -1 in CI or staging.

Out of scope

API routes (trigger and read endpoints) are PR 3/3.

[Vishnu2707]

@emon22-ts - The pipeline is completely failing, please check on the CI tasks.

[TFT444]

Code review

Found 5 issues:

1. started_at is set to completed_at in the INSERT fallback. Scan duration will always be zero on direct calls to save_ci_scan() without a prior create_pending_ci_scan.

openshield/api/models/ci_scan.py

Lines 142 to 146 in 0bde6f8

	# started_at: keep existing value if the pending row
	# already exists; use now() as fallback for direct calls.
	completed_at,
	completed_at,
	status,

2. content_cache is pre-fetched but never passed to rule.scan(). Each of the 4 rules re-fetches all workflow files independently, causing 4x redundant GitHub API calls.

openshield/scanner/ci_engine.py

Lines 126 to 135 in 0bde6f8

	content_cache: dict = {}
	for wf in workflows:
	path = wf.get("path", "")
	if path and path not in content_cache:
	content_cache[path] = self.client.get_workflow_content(path)
	# Build a set of workflow paths that had findings per rule
	for rule in _CI_RULES:
	try:
	findings = rule.scan(self.client, self.owner, self.repo)

3. create_pending_ci_scan() and update_ci_scan_status() have no try/except/rollback. A DB error leaves the pool connection dirty. save_ci_scan() handles this correctly but these two do not.

openshield/api/models/ci_scan.py

Lines 77 to 95 in 0bde6f8

	def create_pending_ci_scan(self, scan_id: str, owner: str, repo: str) -> None:
	"""Insert a ci_scans row in the 'pending' state.
	Called before the engine starts so the scan is visible to callers
	immediately on trigger (PR 3/3 uses this for the 202 response).
	"""
	started_at = datetime.now(timezone.utc)
	conn = self._get_conn()
	with conn.cursor() as cur:
	cur.execute(
	"""
	INSERT INTO ci_scans (scan_id, owner, repo, started_at, status)
	VALUES (%s, %s, %s, %s, 'pending')
	ON CONFLICT (scan_id) DO NOTHING
	""",
	(scan_id, owner, repo, started_at),
	)
	conn.commit()
	logger.info("CIDatabase: created pending CI scan %s for %s/%s", scan_id, owner, repo)

4. az_ci_ skip guard in load_rules() fires before the SeverityContractError / RULE_ID validation block. A malformed CI rule file is silently skipped instead of failing at startup.

openshield/scanner/engine.py

Lines 79 to 95 in 0bde6f8

	if rule_path.stem.startswith("az_ci_"):
	logger.debug("Skipping CI rule %s (requires GitHubClient)", rule_path.name)
	continue
	try:
	spec = importlib.util.spec_from_file_location(rule_path.stem, rule_path)
	module = importlib.util.module_from_spec(spec) # type: ignore[arg-type]
	spec.loader.exec_module(module) # type: ignore[union-attr]
	rule_id = getattr(module, "RULE_ID", None)
	if callable(getattr(module, "scan", None)) and isinstance(rule_id, str) and rule_id:
	declared_severity = getattr(module, "SEVERITY", None)
	if normalize_severity(declared_severity) != declared_severity:
	raise SeverityContractError(f"rule {rule_id} must declare a canonical severity")
	self.rules.append(module)
	logger.info("Loaded rule: %s", rule_id)
	else:
	logger.warning("Rule file %s has no scan() function or RULE_ID — skipped", rule_path.name)
	except SeverityContractError:

5. Raw str(exc) is stored in RuleEvaluation.reason. Psycopg2/network exceptions can contain DSN fragments or hostnames that will be returned by the PR 3/3 API route.

openshield/scanner/ci_engine.py

Lines 143 to 147 in 0bde6f8

	status=EvaluationStatus.ERROR,
	reason_code="RULE_EXCEPTION",
	reason=f"Rule raised an exception: {exc}",
	)
	)

[parthrohit22]

Reviewed 0bde6f8. The split into ci_scans / ci_scan_evaluations makes sense given scans.subscription_id is NOT NULL, and the evaluation constraints mirror rule_evaluations correctly. Three things block, and CI is red.

1. A test removes DATABASE_URL for the rest of the session

test_missing_dsn_raises_before_db_call calls os.environ.pop("DATABASE_URL", None) before entering patch.dict, so the removal is never restored. Every Postgres-backed test that runs after this module then fails at setup with KeyError: 'DATABASE_URL'. That is why Backend Tests is red on this head: test_enrichment_jobs_postgres.py, test_remediation_gate_postgres.py, test_operational_metrics_postgres.py and test_observability.py all fail, and none of them are touched by this PR.

The patch.dict("os.environ", {}, clear=True) block already gives you an empty environment, so the pop can just be deleted. The one at line 120 is inside patch.dict and is fine, but it's redundant for the same reason.

2. Revision ID e1f2a3b4c5d6 is already taken

#352 uses e1f2a3b4c5d6 for graph_schema. Alembic refuses to start with two files sharing a revision ID, so these two can't both land as they are. This PR is the newer one, so please generate a fresh ID (alembic revision or any unused 12-hex string) and update the Revision ID: docstring line.

Also, #326's a8d9c2e4f601 chains off b6d2f8a4c1e7 too, so whichever of the two merges second has to repoint down_revision onto the other or dev ends up with two heads.

3. ci_scans has no tenant scope

Every other table the API reads per tenant carries tenant_id (findings via scans, patterns, the graph tables). ci_scans only has owner/repo, so the PR 3/3 read endpoints will have nothing to scope by, and any authenticated caller would see every repo's CI results. It's much cheaper to add tenant_id (and an index on (tenant_id, owner, repo)) now than to backfill it after the schema ships.

Lint

ruff check fails with F401: unittest.mock.call imported but unused in tests/test_ci_persistence.py:11.

Non-blocking

• create_pending_ci_scan uses ON CONFLICT (scan_id) DO NOTHING, and the upsert in save_ci_scan never updates owner/repo. If PR 3/3 ever reuses a scan_id for a different repo, the results are saved under the old repo's header. Either conflict on (scan_id, owner, repo) or raise when the existing row doesn't match.
• update_ci_scan_status(..., "failed") doesn't set completed_at, so failed scans have no end time.
• The PR description still says the migration chains off 3f59f83a5253; it's b6d2f8a4c1e7 now.
• This PR contains all of #280 (PR 1/3). Reviewing it against dev means re-reviewing the AZ-CI rules too. It would be easier to land #280 first, or retarget this PR at #280's branch.
• SCA (pip-audit) fails on Werkzeug 3.1.8 / multidict 6.8.0. That isn't caused by this PR (dev has the same problem now); the bump is sitting in several other open PRs and should probably land on its own.

What I did not check: I didn't run the migration against Postgres. CI never got that far, for the reason in 1.

[Vishnu2707]

@emon22-ts - please address the failures mentioned above for it to be reviewed.

[emon22-ts]

Thanks for the review @Vishnu2707 @TFT444 @parthrohit22 . Pushed fixes:

1. DATABASE_URL leak: removed both os.environ.pop("DATABASE_URL") calls. The tests no longer touch the real environment.
2. Revision ID: migration is now 4d8a2f6c9b13 (still chains off b6d2f8a4c1e7). Whichever of this and feat(automation): finding lifecycle engine, scan outcome contracts, and pattern detection [1/5] #326 merges second will repoint down_revision; I'll confirm alembic heads shows a single head before merge.
3. Lint: removed the unused call import. ruff check passes.

Non-blocking items: save_ci_scan now raises if a scan_id already belongs to a different owner/repo; failed scans now get completed_at; description updated.

Re tenant_id: I can't find tenant_id in alembic/ or api/ on dev (git grep tenant_id upstream/dev returns nothing), so I don't know which column type, nullability, or JWT claim to follow. Which PR or migration defines the convention? If it's #352, I'll mirror its definition and index. Otherwise I'd add a nullable tenant_id text plus an index on (tenant_id, owner, repo), required on the CIDatabase read methods and run_and_persist(). Tell me if you'd prefer something different.

SCA (pip-audit) fails the same way on dev (Werkzeug/multidict), so it isn't caused by this PR.

This PR is stacked on #280, so I'll rebase once #280 lands.

[TFT444]

Thanks for the lint and migration fixes. The five issues from the previous review are still open though:

1. save_ci_scan passes completed_at for started_at on fresh inserts, causing a zero duration bug.
2. content_cache is built but never passed into rule.scan(), so each rule re-fetches independently.
3. create_pending_ci_scan and update_ci_scan_status have no rollback on error.
4. The az_ci_ skip guard fires before module validation, so malformed files are silently skipped.
5. str(exc) in reason can leak DSN fragments through the API. Use a generic string and log with exc_info=True instead.

Also: run ruff format tests/test_ci_persistence.py to fix the lint failure. And please add a nullable tenant_id column with an index on (tenant_id, owner, repo). Without it any authenticated user can read any org's CI results.