TenantAtlas/docs/audits/legacy-orphaned-truth-audit.md

# Repo-wide Legacy / Orphaned Truth Audit

**Date**: 2026-03-16  
**Scope**: Full codebase — models, migrations, enums, services, jobs, observers, Filament resources, policies, capabilities, badges, tests, factories  
**Method**: Systematic source-of-truth tracing across all layers

---

## Executive Summary

### Top 5 Most Dangerous Stale/Legacy Patterns

1. **`Tenant.app_status` displayed in TenantResource as an active badge despite having ZERO write paths** — admins see a permanently frozen "OK" badge that never reflects real provider health. This is a **dangerous operational lie**.

2. **ProviderConnection dual-status architecture** — four status columns exist (`status`, `health_status`, `consent_status`, `verification_status`). The first two are projections of the latter two, but the ProviderConnectionStateProjector computes values without always persisting them consistently. The UI shows both stacks, creating confusing overlapping semantics.

3. **`backup_sets.item_count` denormalized counter** — actively used in UI and operations but drifts silently when BackupItems are mutated outside the AddPoliciesToBackupSetJob/RemovePoliciesFromBackupSetJob paths (e.g., through RelationManager deletions).

4. **TenantFactory populates stale fields (`app_status='ok'`, `app_client_id=uuid`)** on every test tenant creation, keeping legacy architecture alive in the test suite and masking that these fields are production-dead.

5. **`SyncRestoreRunToOperation.php` orphaned listener** — never called, superseded by `SyncRestoreRunToOperationRun.php`, but its presence creates confusion about which sync path is canonical.

### Cleanup-only Items (Low Risk)
- `Tenant.app_notes` — never written, never displayed, pure dead column
- `tenant_user` legacy pivot table — data migrated to `tenant_memberships`, table still present
- No-op migration file `2026_01_07_142719_create_inventory_items_table.php`

### Items That Can Actively Mislead Operators
- `app_status` badge in TenantResource (**dangerous lie** — frozen at factory default)
- ProviderConnection showing 4 status fields with overlapping semantics (**confusing UX**)
- Badge normalization mismatches (`TenantAppStatus` filter accepts `unknown` but badge has no matching spec → falls back to gray question mark)

### Spec Candidate Themes
- **Spec: Tenant Legacy Identity Cleanup** — remove `app_status`, `app_notes`, clean up `app_client_id`/`app_client_secret` to explicit migration-only fields  
- **Spec: ProviderConnection Status Simplification** — collapse 4 status columns to 2 canonical enums, derive projections live
- **Spec: Denormalized Counter Integrity** — event-driven sync for `backup_sets.item_count`

---

## Findings by Category

---

### Finding 1: Tenant.app_status — Stale Badge Displayed as Truth

**Category**: 1. Stale duplicate state / 5. Dead-end UI  
**Severity**: **CRITICAL**  
**Confidence**: HIGH

**Why this exists**: Early in the project (Dec 2025), `app_status` was added to track tenant app registration health (ok, consent_required, error, unknown). The ProviderConnection model was introduced Jan 2026, superseding this with `consent_status` and `verification_status`.

**Canonical source of truth**: `ProviderConnection.consent_status` + `ProviderConnection.verification_status`

**Stale structure**: `tenants.app_status` column (varchar, default `'unknown'`, indexed)

**Evidence**:
- Migration: `database/migrations/2025_12_11_121623_add_app_fields_to_tenants_table.php`
- Model: `app/Models/Tenant.php` — field is fillable
- Badge domain: `BadgeDomain::TenantAppStatus` maps values (ok, configured, pending, consent_required, error)
- UI display: `app/Filament/Resources/TenantResource.php` line 277 (table column), line 835 (infolist)
- Filter: `app/Filament/Resources/TenantResource.php` line 302 (SelectFilter with hardcoded options)

**Read paths**: TenantResource table column (badge), TenantResource infolist, TenantResource filter  
**Write paths**: **NONE** — zero production code writes to this field after initial migration default

**Why this is dangerous**: An admin viewing the tenant list sees an "App Status: OK" badge on every tenant. This badge is frozen at the factory/migration default. Real provider health has moved to ProviderConnection, but this badge creates false confidence that app registration is healthy.

**Recommended action**: **MUST REMOVE**
1. Remove `app_status` column display from TenantResource (table + infolist + filter)
2. Remove `BadgeDomain::TenantAppStatus` badge domain
3. Drop `app_status` column via migration
4. If admins need this info: derive from ProviderConnection.consent_status live

**Classification**: must remove

---

### Finding 2: Tenant.app_notes — Orphaned Column

**Category**: 1. Stale duplicate state  
**Severity**: LOW  
**Confidence**: HIGH

**Why this exists**: Added alongside `app_status` in Dec 2025 for freeform notes about app registration.

**Stale structure**: `tenants.app_notes` column (text, nullable)

**Read paths**: None — not displayed in any resource or service  
**Write paths**: None — only set to `null` by TenantFactory

**Why this is harmless**: Not displayed anywhere, not read by any code path. Pure dead column.

**Recommended action**: Remove — drop column via migration  
**Classification**: can remove

---

### Finding 3: Tenant Legacy Identity Fields (app_client_id, app_client_secret)

**Category**: 6. Partially completed migrations  
**Severity**: MEDIUM  
**Confidence**: HIGH

**Why this exists**: Before ProviderConnection + ProviderCredential existed, tenants stored app registration credentials directly. The provider migration system (Spec 081) introduced a classification layer that reads these fields to determine if a tenant has legacy identity that needs migration review.

**Canonical source of truth**: `ProviderCredential.client_id` / `ProviderCredential.client_secret`

**Legacy structure**: `tenants.app_client_id`, `tenants.app_client_secret` (encrypted), `tenants.app_certificate_thumbprint`

**Read paths**:
- `ProviderConnectionMigrationClassificationService` — reads for legacy identity detection
- Guard tests — `NoPlatformCredentialFallbackTest`, `NoLegacyTenantProviderFallbackTest`
- `MembershipAuditLogTest` — asserts `app_client_secret` is NOT in audit metadata

**Write paths**: Only TenantFactory (sets `app_client_id` to `fake()->uuid()` on every creation)

**Why this matters**: These fields are intentionally read-only during the provider migration cutover. The classification service uses them to detect legacy identity and flag `migration_review_required` on ProviderConnection. They are NOT duplicate state — they are migration-source data.

**Recommended action**: Keep temporarily — explicitly mark as `@deprecated` in model, add PHPDoc explaining migration-only purpose. Schedule removal after all tenants pass migration review.  
**Classification**: should remove (after migration completion)

---

### Finding 4: ProviderConnection Dual Status Architecture

**Category**: 1. Stale duplicate state / 3. Mapping drift  
**Severity**: HIGH  
**Confidence**: HIGH

**Why this exists**: ProviderConnection was created (Jan 2026) with varchar `status` and `health_status` fields. In Mar 2026, enum-backed `consent_status` and `verification_status` were added as the new source of truth. The old varchar fields were kept as "projected" summaries for UI display.

**Canonical source of truth**: `consent_status` (ProviderConsentStatus enum) + `verification_status` (ProviderVerificationStatus enum)

**Stale structure**: `status` (varchar: connected/needs_consent/error/disabled) + `health_status` (varchar: ok/degraded/down/unknown)

**Evidence**:
- Original migration: `database/migrations/2026_01_24_000001_create_provider_connections_table.php`
- New enum columns: `database/migrations/2026_03_13_000001_add_provider_identity_fields_to_provider_connections.php`
- Projector: `app/Services/Providers/ProviderConnectionStateProjector.php`
- UI reads BOTH stacks: `app/Filament/Resources/ProviderConnectionResource.php`

**Read paths**: ProviderConnectionResource table (4 columns), filters (2 filters on varchar fields), badge system (2 badge domains)  
**Write paths**: ProviderConnectionHealthCheckJob calls projector → writes both stacks atomically

**Why this is dangerous**:
1. Three incompatible status vocabularies displayed together (varchar status, varchar health, enum consent, enum verification)
2. Badge normalization functions hidden in BadgeCatalog translate between vocabularies
3. If projector fails or isn't called on a code path, varchar fields drift from enum truth
4. ProviderConnectionStateProjector does NOT always persist projected values
5. UI filters use varchar vocabulary, which breaks if normalization rules change

**Recommended action**: **Finish cutover** — migrate UI to read `consent_status` and `verification_status` directly. Deprecate varchar `status` and `health_status`. Eventually drop columns.  
**Classification**: should remove

---

### Finding 5: backup_sets.item_count Denormalized Counter Drift

**Category**: 4. Read paths without credible write paths  
**Severity**: MEDIUM  
**Confidence**: HIGH

**Why this exists**: `item_count` is a denormalized integer stored on BackupSet for quick display without counting related BackupItems.

**Canonical source of truth**: `BackupSet::items()->count()` (relationship count)

**Stale structure**: `backup_sets.item_count` column (unsignedInteger, default 0)

**Read paths**: BackupSetResource table column, BackupSetResource detail page (2x), RunBackupScheduleJob, RemovePoliciesFromBackupSetJob  
**Write paths**: AddPoliciesToBackupSetJob, RemovePoliciesFromBackupSetJob, BackupService (all call `$backupSet->items()->count()` to sync)

**Why this matters**: The write path re-counts from DB, which is correct. But if BackupItems are deleted through the BackupItemsRelationManager or soft-delete cascading, and the parent BackupSet isn't re-synced through the job infrastructure, the counter drifts silently. Currently no event/observer recounts on BackupItem delete.

**Recommended action**: Either add an observer on BackupItem to recount parent, or replace with `withCount('items')` in queries and drop the denormalized column.  
**Classification**: should remove (or add sync observer)

---

### Finding 6: SyncRestoreRunToOperation.php — Orphaned Listener

**Category**: 8. Jobs/events/listeners that no longer matter  
**Severity**: LOW  
**Confidence**: HIGH (confirmed via Spec 087)

**Why this exists**: Early listener for RestoreRun → OperationRun sync. Superseded by `SyncRestoreRunToOperationRun.php` which is called directly from the RestoreRunObserver.

**Stale structure**: `app/Listeners/SyncRestoreRunToOperation.php`

**Read paths**: None — never imported, never registered  
**Write paths**: N/A — never instantiated

**Evidence**: Zero grep matches for class name. Spec 087 analysis report (line 616) flagged it as "Low risk; verify/keep during transition."

**Recommended action**: **Remove** — Spec 087 is complete, transition is done  
**Classification**: can remove

---

### Finding 7: TenantFactory Populates Stale Fields

**Category**: 9. Test suite preserving dead architecture  
**Severity**: MEDIUM  
**Confidence**: HIGH

**Why this exists**: Factory was created with the original Tenant schema and never cleaned up as fields became stale.

**Stale structure**: `database/factories/TenantFactory.php` lines 40-44, 49

**Evidence**:
```php
'app_client_id' => fake()->uuid(),  // Legacy: only read for migration classification
'app_status' => 'ok',               // Dead: never updated in production
'app_notes' => null,                // Dead: never written or read
'rbac_status' => 'ok',              // Hardcoded: never reflects real health checks
```

**Why this matters**: Every `Tenant::factory()->create()` call populates dead fields with plausible-looking values. Tests pass even though these values never match production behavior. `app_status='ok'` in tests masks that the field is always `'unknown'` in production (migration default). `rbac_status='ok'` prevents testing RBAC degradation paths.

**Recommended action**: 
1. Remove `app_status` and `app_notes` from factory definition
2. Make `app_client_id` nullable in factory (set only when testing migration classification)
3. Set `rbac_status` to `null` in default definition; use named states for health scenarios

**Classification**: should remove (update factory)

---

### Finding 8: Badge Normalization Mismatches

**Category**: 3. Mapping drift / badge drift  
**Severity**: MEDIUM  
**Confidence**: HIGH

**Why this exists**: Badge normalization functions translate between different state vocabularies (enum values → display values). Over time, the translation rules have diverged from the actual value sets.

**Evidence**:
- `TenantAppStatus` filter accepts `unknown` but badge domain has no explicit mapping → falls back to `BadgeSpec::unknown()` (gray question mark)
- `ProviderConnectionStatus` badge normalizes 7+ input values into 3 display states; drift if new enum cases added
- `ProviderConnectionHealth` badge normalizes `healthy` → `ok` and `blocked`/`error` → `down`; these translations are buried in badge catalog, not documented

**Why this matters**: When an admin sees a gray question mark badge, they cannot distinguish "this record has a genuinely unknown status" from "this record has a status value that the badge system doesn't recognize." This masks data quality issues.

**Recommended action**: Add exhaustive badge mapping tests (dataset-driven) ensuring every enum case has an explicit badge spec. Remove `BadgeSpec::unknown()` fallback for production badge domains — force all values to be explicitly mapped.  
**Classification**: should remove (fallback masking)

---

### Finding 9: tenant_user Legacy Pivot Table

**Category**: 6. Partially completed migrations  
**Severity**: LOW  
**Confidence**: HIGH

**Why this exists**: Original many-to-many pivot for tenant ↔ user. Migrated to `tenant_memberships` table (richer model with role, source, created_by) in Jan 2026.

**Old world**: `tenant_user` pivot table  
**New world**: `tenant_memberships` table (TenantMembership model)  
**What still references old**: Data was backfilled via `2026_01_25_023708_backfill_tenant_memberships_from_tenant_user.php`. No model references `tenant_user`.

**What prevents deletion**: No drop migration exists. Table is present but unused.

**Recommended action**: Create drop migration for `tenant_user` table. Add architecture guard test.  
**Classification**: can remove

---

### Finding 10: EntraGroupPolicy Bypasses Capability Layer

**Category**: 7. Orphaned policies / security-relevant  
**Severity**: MEDIUM  
**Confidence**: HIGH

**Why this exists**: EntraGroupPolicy was created for the EntraGroupResource but only checks `canAccessTenant()` — it does not check any capability constants.

**Evidence**: `app/Policies/EntraGroupPolicy.php` — `viewAny()` and `view()` methods call `$user->canAccessTenant()` only.

**Why this matters**: All other policies enforce capability checks (e.g., `TENANT_FINDINGS_VIEW`, `WORKSPACE_BASELINES_MANAGE`). EntraGroupPolicy is the only read-access policy that skips the capability layer entirely. While EntraGroups are read-only, this inconsistency could lead to copy-paste errors if new methods are added.

**Recommended action**: Add appropriate capability check (e.g., `TENANT_VIEW`) or document the intentional bypass.  
**Classification**: should remove (the bypass, not the policy)

---

### Finding 11: ProviderConnectionStateProjector Untested

**Category**: 9. Test suite preserving dead architecture (gap variant)  
**Severity**: MEDIUM  
**Confidence**: HIGH

**Why this exists**: The projector is a critical bridge between the new enum status sources and the old varchar display fields. Despite being called from 5+ production paths, it has no dedicated test coverage.

**Evidence**: `app/Services/Providers/ProviderConnectionStateProjector.php` — called by TenantOnboardingController, AdminConsentCallbackController, ManagedTenantOnboardingWizard, CreateProviderConnection, ProviderConnectionHealthCheckJob.

**Why this matters**: If the projection logic breaks (e.g., a new enum case is added to `ProviderConsentStatus` without updating the projector), the derived `status` and `health_status` fields silently produce wrong values. No test would catch this.

**Recommended action**: Create dataset-driven unit test covering all enum combinations → projected varchar values.  
**Classification**: must address (test gap for critical infrastructure)

---

### Finding 12: RestoreRunStatus Legacy Enum Cases

**Category**: 3. Mapping drift / enum drift  
**Severity**: LOW  
**Confidence**: HIGH

**Why this exists**: `RestoreRunStatus` enum includes `Aborted` and `CompletedWithErrors` cases labeled as `(legacy)` in the enum inventory. These were from an earlier restore workflow.

**Evidence**: `app/Support/RestoreRunStatus.php` — cases `Aborted` and `CompletedWithErrors` exist but are never written by current restore execution code.

**Why this matters**: These cases still have badge mappings and can appear in filters, but no production code path produces them. They are backward-compatibility noise that should be formally deprecated.

**Recommended action**: Mark with `@deprecated` annotation. Keep for now (DB may contain historical records with these values) but exclude from filters and creation logic.  
**Classification**: can remove (from active UX, keep in enum for deserialization)

---

### Finding 13: TENANT_ROLE_MAPPING Capabilities — Defined but No UI

**Category**: 7. Orphaned capabilities  
**Severity**: LOW  
**Confidence**: MEDIUM

**Why this exists**: `TENANT_ROLE_MAPPING_VIEW` and `TENANT_ROLE_MAPPING_MANAGE` are defined in `Capabilities.php` and mapped in role capability resolvers, but no Filament resource or page enforces them. The TenantRoleMapping model exists with a migration but has no admin UI.

**Evidence**: `app/Support/Auth/Capabilities.php` — constants defined. Tests confirm capability resolver maps them correctly. No Filament resource references them.

**Why this matters**: This is intentional WIP infrastructure (Spec 062, T028 marked optional). Not dangerous, just incomplete.

**Recommended action**: Keep — this is staged infrastructure. Add a comment documenting the planned UI.  
**Classification**: keep (intentional WIP)

---

### Finding 14: Direct Graph Calls Bypassing ProviderGateway

**Category**: 2. Legacy abstractions still referenced  
**Severity**: MEDIUM  
**Confidence**: HIGH

**Why this exists**: Two locations in TenantResource.php make direct `$graph->request('GET', ...)` calls instead of using the ProviderGateway abstraction layer.

**Evidence**:
- `app/Filament/Resources/TenantResource.php` line ~1571: `$graph->request('GET', 'deviceManagement/roleDefinitions', ...)`
- `app/Filament/Resources/TenantResource.php` line ~1692: `$graph->request('GET', 'groups', ...)`

**Why this matters**: All other Graph calls go through ProviderGateway, which handles credential resolution, contract validation, and error mapping. These direct calls bypass those protections and use an inconsistent error handling pattern.

**Recommended action**: Refactor to use ProviderGateway or a dedicated service method.  
**Classification**: should remove (the direct calls)

---

### Finding 15: AlertDeliveryPolicy Incomplete Methods

**Category**: 7. Orphaned policies  
**Severity**: LOW  
**Confidence**: MEDIUM

**Evidence**: `app/Policies/AlertDeliveryPolicy.php` — only `viewAny()` and `view()` exist. No `create()`, `update()`, `delete()`. AlertDelivery is a read-only model (deliveries are created by jobs, not users), so this is likely intentional but should be documented.

**Recommended action**: Add `create()`, `update()`, `delete()` methods returning `false` to make read-only intent explicit. Or document in PHPDoc.  
**Classification**: can remove (the ambiguity, not the policy)

---

## Cross-cutting Architectural Themes

### Theme A: Denormalized Status Shadow Fields
**Pattern**: A canonical source of truth exists (enum column, service output, relationship count) but a denormalized varchar/integer copy is persisted for query performance or historical reasons, then displayed in UI without lifecycle maintenance.

**Instances**: `Tenant.app_status`, `ProviderConnection.status`/`health_status`, `backup_sets.item_count`

**Root cause**: Columns were added before the canonical source existed, then the canonical source was introduced without deprecating the shadow field.

**Fix pattern**: For each shadow field, choose one of:
- Live derivation (compute on read via accessor or query join)
- Event-driven sync (observer/listener updates shadow on canonical change)
- Remove entirely (if no query performance justification)

### Theme B: Incomplete Provider Identity Cutover
**Pattern**: Tenant was the original identity holder (app_client_id, app_client_secret, app_status). ProviderConnection + ProviderCredential now own identity. Migration classification exists but the old Tenant columns remain populated (by factory) and displayed (by TenantResource).

**What blocks full cutover**: The classification service still reads `Tenant.app_client_id` to detect legacy identity. Until all tenants pass migration review, these fields serve as migration input.

**Residual risk**: Factory continues populating legacy fields, tests assert against them, TenantResource displays `app_status` badge.

### Theme C: Badge Normalization as Hidden Translation Layer
**Pattern**: Badge domains contain normalization functions that translate between different value vocabularies. These are undocumented, untested, and drift-prone.

**Instances**: TenantAppStatus normalizer, ProviderConnectionStatus normalizer, ProviderConnectionHealth normalizer.

**Fix pattern**: Exhaustive enum-to-badge mapping tests. Remove `BadgeSpec::unknown()` fallback for production domains. Move normalization out of badge catalog into explicit enum methods.

### Theme D: Test Suite Preserving Dead Architecture
**Pattern**: Factories populate stale fields with plausible values, making tests pass even though those fields are production-dead. Guard tests exist (excellent practice) but factory defaults create a false baseline.

**Instances**: TenantFactory sets `app_status='ok'`, `rbac_status='ok'`, generates `app_client_id`.

**Fix pattern**: Factory defaults should match production reality. Use named factory states for legacy-identity-specific test scenarios.

### Theme E: Projection Without Test Coverage
**Pattern**: Critical state-computation services (ProviderConnectionStateProjector) translate between enum sources and derived display fields but have no dedicated test coverage.

**Risk**: A new enum case added without projector update silently produces wrong UI state.

**Fix pattern**: Dataset-driven exhaustive projector tests covering all enum combinations.

---

## Spec Candidate Recommendations

### Spec Candidate 1: Tenant Legacy Identity Cleanup
**Scope**: Remove `app_status`, `app_notes` column and UI surfaces. Mark `app_client_id`/`app_client_secret` as `@deprecated` migration-only fields. Clean TenantFactory.

**Tasks**:
1. Remove `app_status` from TenantResource (table, infolist, filter)
2. Remove `BadgeDomain::TenantAppStatus` and related badge files
3. Migration: drop `app_status`, `app_notes` columns + index
4. Update TenantFactory: remove `app_status`, `app_notes` from definition
5. Add `@deprecated` PHPDoc to `app_client_id`, `app_client_secret` on Tenant model
6. Update tests that reference `app_status`
7. Drop `tenant_user` legacy pivot table

**Findings covered**: #1, #2, #3, #7, #9

---

### Spec Candidate 2: ProviderConnection Status Simplification
**Scope**: Collapse 4 status columns to 2 canonical enums. UI reads enums directly. Drop varchar projection columns.

**Tasks**:
1. Migrate TenantResource and ProviderConnectionResource to read `consent_status`/`verification_status` directly
2. Update badge domains to map enum values (not projected varchars)
3. Update filters to use enum values
4. Create projector test suite (exhaustive enum combinations)
5. Deprecate, then drop `status` and `health_status` varchar columns
6. Remove normalization functions from BadgeCatalog

**Findings covered**: #4, #8, #11

---

### Spec Candidate 3: Denormalized Counter Integrity
**Scope**: Fix `backup_sets.item_count` drift. Either add observer for sync or replace with live count.

**Tasks**:
1. Add BackupItem observer that recounts parent BackupSet `.item_count` on create/delete/restore
2. Or: replace all reads of `item_count` with `withCount('items')` and drop the column
3. Add test coverage for count drift scenario

**Findings covered**: #5

---

### Spec Candidate 4: Orphaned Code Removal (Quick Wins)
**Scope**: Remove clearly dead code identified in this audit.

**Tasks**:
1. Delete `app/Listeners/SyncRestoreRunToOperation.php`
2. Add architecture guard test confirming it's gone
3. Clean up `RestoreRunStatus::Aborted` and `CompletedWithErrors` from active filters (keep in enum)
4. Add capability check to `EntraGroupPolicy` or document bypass
5. Add explicit deny methods to `AlertDeliveryPolicy`
6. Refactor 2 direct Graph calls in TenantResource to use ProviderGateway

**Findings covered**: #6, #10, #12, #14, #15