feat(graph): evidence graph foundation - resource inventory schema and scan wiring

[TFT444]

Summary

Lays the foundation for the OpenShield attack graph (issue #331). Three self-contained changes:

• DB schema (alembic/versions/e1f2a3b4c5d6_graph_schema.py): adds graph_nodes, graph_edges, and finding_graph_nodes tables with cascade deletes, unique constraints, and full downgrade support
• Snapshot bridge (scanner/graph/snapshot_bridge.py): wraps ArgInventoryClient into a single collect_snapshot() call for use inside ScanEngine; failure is non-fatal (returns None so rules fall back to direct SDK)
• Engine wiring (scanner/engine.py): collects one InventorySnapshot per scan before the rule loop and passes it as an optional third argument to each rule's scan() call; existing two-arg rules keep working via TypeError fallback; result dict gains snapshot_id and snapshot_status

What is not in this PR

Node population, edge detection, path traversal, and API endpoints come in PRs 2/3 and 3/3 (issues #332 and #333).

Test coverage

• tests/test_graph_migration.py: schema structure tests (require live PostgreSQL)
• tests/test_graph_snapshot_bridge.py: 4 unit tests covering success, exception, FAILED, and PARTIAL snapshot status
• tests/test_graph_engine_integration.py: 4 unit tests covering snapshot wiring, None fallback, snapshot passed to rule, and legacy two-arg rule compatibility

Full suite: 1361 passed, 9 skipped. The 2 pre-existing failures (test_rules_aks_enterprise, test_subscription_authorization) are not introduced by this branch.

Closes part of #331.

[github-actions]

Dependency Review

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

Scanned Files

None

[parthrohit22]

Thanks @TFT444, splitting the graph work into three stacked PRs makes this a lot easier to follow, and keeping snapshot collection non-fatal is the right call.

I have to request changes though, because as wired today the snapshot is never collected in a real scan.

1. AzureClient has no tenant_id, so collect_snapshot() always returns None

snapshot_bridge.collect_snapshot does getattr(client, "tenant_id", None), but AzureClient.__init__ only sets subscription_id and credential (scanner/azure_client.py:51). Nothing else sets a tenant on it either: ScanEngine builds it as AzureClient(subscription_id). So in production we always go down the "missing tenant_id or credential" branch and return None. I checked it against the real class instead of a MagicMock:

snapshot_bridge: AzureClient missing tenant_id or credential — skipping ARG collection
has tenant_id: False -> snapshot: None

The tests don't catch this because every one of them uses MagicMock(), which will happily return a truthy tenant_id. Once that happens, #353 and #354 never run either. Could you resolve the tenant explicitly (from AZURE_TENANT_ID, or from the credential/subscription lookup) and add one test that builds a real AzureClient with a stub credential?

2. Migration head collision with #325

e1f2a3b4c5d6 chains from 3f59f83a5253, and so does #325's first revision (e4f7a9b2c6d8). Whichever of the two merges second will leave alembic heads with two heads. #325 is approved and close to merging, so it's probably simplest to plan on rebasing this onto d4a8c1e6b2f9 once it lands.

Smaller things (non-blocking)

• The except TypeError fallback in run_scan also catches a TypeError raised inside a rule that already accepts snapshot. That rule then runs a second time with two args, which duplicates its Azure calls and hides the original error. Checking the signature once with inspect.signature when rules load would avoid that.
• tests/test_graph_migration.py isn't gated on DATABASE_URL like the other Postgres tests, and its module teardown downgrades the shared test database to 3f59f83a5253. Once more revisions sit above this one, that downgrade will drop tables other test modules depend on. The throwaway-database pattern in test_scan_admission_migration_postgres.py would keep it isolated.
• The configure_logging / fileConfig(disable_existing_loggers=False) changes look fine, but they're unrelated to the graph schema. Worth a line in the description saying why they're here.

Happy to take another look once the tenant wiring is sorted.

[parthrohit22]

Heads-up @TFT444: #325 just merged into dev, and it touches this series in two ways.

1. Migration parent. dev's Alembic head is now d4a8c1e6b2f9. e1f2a3b4c5d6 still chains from 3f59f83a5253, so merging as-is would leave two heads and alembic upgrade head would fail. Please rebase and point down_revision at the current head. feat(automation): finding lifecycle engine, scan outcome contracts, and pattern detection [1/5] #326 and fix(compliance): make framework reports evidence-based and non-certifying #310 have the same issue, so whichever lands first takes d4a8c1e6b2f9 and the next one chains onto it.
2. Duplicate revision ID. Your feat(automation): finding lifecycle engine, scan outcome contracts, and pattern detection [1/5] #326 also uses e1f2a3b4c5d6 as its revision ID. Alembic won't start if two files share an ID, so one of them needs a new one.

#353 and #354 stack on this, so they'll need the same rebase. When you request review again, could you check that alembic heads returns a single head on top of current dev? That's the quickest way to catch this. Thanks!

[TFT444]

All blocking items addressed:

1. tenant_id on AzureClient: Added optional tenant_id parameter to AzureClient.__init__() with AZURE_TENANT_ID env var fallback. snapshot_bridge.collect_snapshot() now always gets a valid tenant in production. Tests added for env var, explicit override, and None-when-unset cases.
2. Migration head collision: Updated down_revision in e1f2a3b4c5d6_graph_schema.py to ('3f59f83a5253', 'd4a8c1e6b2f9') to merge both dev heads introduced by fix(core): harden scan durability and idempotency (#303) #325.
3. TypeError guard: Replaced bare except TypeError with inspect.signature check at call time so TypeErrors raised inside rule logic are no longer silently swallowed.

[parthrohit22]

Thanks @TFT444. The tenant wiring and the inspect.signature change both do what I asked. The AzureClient tests now construct the real class, so a missing tenant can't hide behind a MagicMock any more.

One blocker came in with the migration change, and it's why Backend Tests is red on this head.

Blocker: the merge point includes an ancestor of its other parent

down_revision = ("3f59f83a5253", "d4a8c1e6b2f9"). But 3f59f83a5253 is already an ancestor of d4a8c1e6b2f9: #325 rebased its chain onto it before merging. So dev had a single head, and there was nothing to merge. Listing both parents breaks two things. I checked each against a clean PostgreSQL 16 database on this head:

$ alembic upgrade head      -> ok
$ alembic downgrade -1
ERROR [alembic.util.messaging] Ambiguous walk
FAILED: Ambiguous walk

tests/test_alembic_migrations.py::test_single_alembic_head also fails: found ['e1f2a3b4c5d6', 'd4a8c1e6b2f9']. The test's static parser reads the first ID in the tuple, so d4a8c1e6b2f9 looks like an orphaned head. tests/test_graph_migration.py::test_downgrade_removes_tables errors for the same reason.

Fix: use a plain single parent:

down_revision: Union[str, Sequence[str], None] = "d4a8c1e6b2f9"

and change Revises: in the docstring to match. I applied exactly that locally, and got upgrade → downgrade -1 → upgrade clean, test_single_alembic_head passing, and test_graph_migration.py passing.

After that, 4 admission-migration tests still fail

tests/test_scan_admission_migration_postgres.py on dev pins _HEAD = "d4a8c1e6b2f9". Any PR that adds a migration fails these four tests (...reaches_a_single_head and the three ...reruns... / ...rebuilds... tests): assert 'e1f2a3b4c5d6' == 'd4a8c1e6b2f9'. #310 already fixes the test to resolve the head from the revision graph (ScriptDirectory.from_config(...).get_current_head()). Either cherry-pick that one-line change or land after #310.

Merge order with #310 and #326

• #310 chains from the same parent. Its migration 3a76ff935bf6 also chains from d4a8c1e6b2f9, so whichever of #310 or #352 merges second has to repoint to the other's revision. #310 is waiting only on a re-review, so it will probably land first. Worth planning for.
• Duplicate revision ID. #326 still uses e1f2a3b4c5d6 as its own revision ID (e1f2a3b4c5d6_finding_lifecycle.py). Alembic refuses to start with two files sharing an ID, so one of them needs a new one before both can exist on dev.

Non-blocking (from my first review, still open)

• test_graph_migration.py downgrades the shared test database to 3f59f83a5253 in the middle of a run. With #325 on dev, that drops the lease, admission and enrichment tables before re-upgrading. It passes today, but it will fight any test module that runs alongside it. The throwaway-database pattern in test_scan_admission_migration_postgres.py avoids that.
• The import os inside AzureClient.__init__ belongs at module level.

Fix the down_revision, sort out the admission-test head, and this is ready for another look.

[TFT444]

Both blockers from your review are already fixed on the current head (7996820):

1. down_revision fixed — commit c117f8b changed it to a single string "d4a8c1e6b2f9". The Revises docstring was updated to match. alembic upgrade/downgrade -1/upgrade round-trip is clean.

2. _HEAD resolved dynamically — commit 23d731f replaced the hardcoded string with a _resolve_head() function using ScriptDirectory.from_config. The four admission-migration tests should now pass when a new migration is added.

These were pushed a few days ago but the review hasn't been updated since. Could you take another look at the current head? @parthrohit22

[parthrohit22]

Thanks @TFT444. I re-checked the current head (7996820). Both blockers from my earlier reviews are fixed:

• tenant_id: AzureClient now sets it from the argument or AZURE_TENANT_ID, and the tests build the real class. render.yaml and docker-compose already set that variable.
• down_revision is a single parent and the admission test resolves the head dynamically.

One new blocker, caused by dev moving since your last push.

Blocker: migration no longer merges cleanly onto dev

#310 merged after your last push. Its migration 3a76ff935bf6 also chains from d4a8c1e6b2f9, so dev now has two heads once this PR is merged. I merged the PR head onto current dev in a scratch copy and ran it against PostgreSQL:

$ alembic upgrade head
ERROR: Multiple head revisions are present for given argument 'head'

This is not only a test failure. startup.sh runs "alembic upgrade head", so this would break deploys. The suite also errors at collection, because the admission test's head lookup raises.

CI is green only because it last ran before #310 merged.

Fix: set down_revision to "3a76ff935bf6" and update the "Revises:" line in the docstring.

Heads-up: my #364 also chains from 3a76ff935bf6. Whichever of the two lands second repoints onto the other.

Also blocks the series: duplicate revision ID in #326

#326 still uses e1f2a3b4c5d6 as its revision ID, the same as this PR, and its down_revision is d8e4f6a1b2c3, which is well behind. Alembic refuses to start with two files sharing an ID, so one of them needs a new ID before both can sit on dev. This is for #326, but it decides merge order.

Non-blocking

1. Case-sensitive uniqueness. uq_graph_nodes_tenant_resource is on (tenant_id, resource_id). Azure resource IDs are case-insensitive, so the same resource could become two nodes if the casing differs between ARG and a rule finding. Are IDs lowercased before insert? Worth settling before #353 populates the table.
2. Signature check. run_scan passes the snapshot when scan() has 3 or more parameters. All 144 rules take exactly 2, so it is safe today. Checking for a parameter named snapshot (or *args) would be more robust.
3. Every scan now queries Azure Resource Graph, before any rule uses the snapshot, with no timeout. Failure is handled, but it adds latency and needs Reader access. A timeout or a feature flag until #353 lands would limit the impact. The snapshot_id is returned but not stored anywhere yet.
4. tests/test_graph_migration.py still modifies the shared test database and is not skipped when DATABASE_URL is unset (from my first review).
5. The configure_logging and fileConfig changes look fine but are unrelated to the graph. One line in the description would help.

What I did not check: I did not run collect_snapshot against a real Azure subscription.

Once down_revision points at 3a76ff935bf6, this looks good to me. Happy to re-check quickly.

[TFT444]

@parthrohit22 all requested changes are addressed: down_revision points to 3a76ff935bf6 (single parent, not a tuple), import os is at module level, and the admission test resolves the head dynamically. Could you re-review when you get a chance? Thanks.

[TFT444]

@parthrohit22 Migration chain blocker is fixed (down_revision is now a single string). Please re-review when you get a chance.