From 85e4bd75f815a12dfdddf96ae8a2777ff00432f9 Mon Sep 17 00:00:00 2001 From: Ahmed Darrazi Date: Sat, 10 Jan 2026 00:54:40 +0100 Subject: [PATCH] fix: harden 042 requirements gate --- .../Inventory/DependencyExtractionService.php | 30 +++++----- .../Inventory/InventorySyncService.php | 17 ++++-- .../components/dependency-edges.blade.php | 16 ++++- .../checklists/requirements.md | 8 +-- .../DependencyExtractionFeatureTest.php | 59 +++++++++++++++++++ .../Feature/InventoryItemDependenciesTest.php | 40 ++++++++++++- .../Unit/DependencyExtractionServiceTest.php | 31 +++++++--- 7 files changed, 165 insertions(+), 36 deletions(-) diff --git a/app/Services/Inventory/DependencyExtractionService.php b/app/Services/Inventory/DependencyExtractionService.php index ac84e88..54ad580 100644 --- a/app/Services/Inventory/DependencyExtractionService.php +++ b/app/Services/Inventory/DependencyExtractionService.php @@ -7,6 +7,7 @@ use App\Support\Enums\RelationshipType; use Illuminate\Support\Arr; use Illuminate\Support\Collection; +use Illuminate\Support\Facades\Log; class DependencyExtractionService { @@ -16,11 +17,12 @@ class DependencyExtractionService * * @param array $policyData */ - public function extractForPolicyData(InventoryItem $item, array $policyData): void + public function extractForPolicyData(InventoryItem $item, array $policyData): array { + $warnings = []; $edges = collect(); - $edges = $edges->merge($this->extractAssignedTo($item, $policyData)); + $edges = $edges->merge($this->extractAssignedTo($item, $policyData, $warnings)); $edges = $edges->merge($this->extractScopedBy($item, $policyData)); // Enforce max 50 outbound edges by priority: assigned_to > scoped_by > others @@ -57,13 +59,15 @@ public function extractForPolicyData(InventoryItem $item, array $policyData): vo ['metadata', 'updated_at'] ); } + + return $warnings; } /** * @param array $policyData * @return Collection> */ - private function extractAssignedTo(InventoryItem $item, array $policyData): Collection + private function extractAssignedTo(InventoryItem $item, array $policyData, array &$warnings): Collection { $assignments = Arr::get($policyData, 'assignments'); if (! is_array($assignments)) { @@ -93,19 +97,15 @@ private function extractAssignedTo(InventoryItem $item, array $policyData): Coll ], ]; } else { - // Unresolved/unknown target → mark missing - $edges[] = [ - 'tenant_id' => (int) $item->tenant_id, - 'source_type' => 'inventory_item', - 'source_id' => (string) $item->external_id, - 'target_type' => 'missing', - 'target_id' => null, - 'relationship_type' => RelationshipType::AssignedTo->value, - 'metadata' => [ - 'raw_ref' => $assignment, - 'last_known_name' => null, - ], + $warning = [ + 'type' => 'unsupported_reference', + 'policy_id' => (string) ($policyData['id'] ?? $item->external_id), + 'raw_ref' => $assignment, + 'reason' => 'unsupported_assignment_target_shape', ]; + + $warnings[] = $warning; + Log::info('Unsupported reference shape encountered', $warning); } } diff --git a/app/Services/Inventory/InventorySyncService.php b/app/Services/Inventory/InventorySyncService.php index 3ca113b..79da1a7 100644 --- a/app/Services/Inventory/InventorySyncService.php +++ b/app/Services/Inventory/InventorySyncService.php @@ -108,6 +108,7 @@ private function executeRun(InventorySyncRun $run, Tenant $tenant, array $normal $errors = 0; $errorCodes = []; $hadErrors = false; + $warnings = []; try { $typesConfig = $this->supportedTypeConfigByType(); @@ -186,8 +187,11 @@ private function executeRun(InventorySyncRun $run, Tenant $tenant, array $normal // Extract dependencies if requested in selection $includeDeps = (bool) ($normalizedSelection['include_dependencies'] ?? true); if ($includeDeps) { - app(\App\Services\Inventory\DependencyExtractionService::class) - ->extractForPolicyData($item, $policyData); + $warnings = array_merge( + $warnings, + app(\App\Services\Inventory\DependencyExtractionService::class) + ->extractForPolicyData($item, $policyData) + ); } } } @@ -198,7 +202,9 @@ private function executeRun(InventorySyncRun $run, Tenant $tenant, array $normal 'status' => $status, 'had_errors' => $hadErrors, 'error_codes' => array_values(array_unique($errorCodes)), - 'error_context' => null, + 'error_context' => [ + 'warnings' => array_values($warnings), + ], 'items_observed_count' => $observed, 'items_upserted_count' => $upserted, 'errors_count' => $errors, @@ -207,11 +213,14 @@ private function executeRun(InventorySyncRun $run, Tenant $tenant, array $normal return $run->refresh(); } catch (Throwable $throwable) { + $errorContext = $this->safeErrorContext($throwable); + $errorContext['warnings'] = array_values($warnings); + $run->update([ 'status' => InventorySyncRun::STATUS_FAILED, 'had_errors' => true, 'error_codes' => ['unexpected_exception'], - 'error_context' => $this->safeErrorContext($throwable), + 'error_context' => $errorContext, 'items_observed_count' => $observed, 'items_upserted_count' => $upserted, 'errors_count' => $errors + 1, diff --git a/resources/views/filament/components/dependency-edges.blade.php b/resources/views/filament/components/dependency-edges.blade.php index cb2240e..fcaec87 100644 --- a/resources/views/filament/components/dependency-edges.blade.php +++ b/resources/views/filament/components/dependency-edges.blade.php @@ -38,11 +38,23 @@ $name = $edge['metadata']['last_known_name'] ?? null; $targetId = $edge['target_id'] ?? null; $display = $name ?: ($targetId ? ("ID: ".substr($targetId,0,6)."…") : 'Unknown'); + + $missingTitle = 'Missing target'; + if (is_string($name) && $name !== '') { + $missingTitle .= ". Last known: {$name}"; + } + $rawRef = $edge['metadata']['raw_ref'] ?? null; + if ($rawRef !== null) { + $encodedRef = json_encode($rawRef); + if (is_string($encodedRef) && $encodedRef !== '') { + $missingTitle .= '. Ref: '.\Illuminate\Support\Str::limit($encodedRef, 200); + } + } @endphp
  • - {{ $display }} + {{ $display }} @if ($isMissing) - Missing + Missing @endif
    • @endforeach diff --git a/specs/042-inventory-dependencies-graph/checklists/requirements.md b/specs/042-inventory-dependencies-graph/checklists/requirements.md index ca8397e..0282beb 100644 --- a/specs/042-inventory-dependencies-graph/checklists/requirements.md +++ b/specs/042-inventory-dependencies-graph/checklists/requirements.md @@ -13,7 +13,7 @@ ## Constitution Gates - [x] Tenant isolation: all reads/writes tenant-scoped - [x] Automation is idempotent & observable: unique key + upsert + run records + stable error codes - [x] Data minimization & safe logging: no secrets/tokens; avoid storing raw payloads outside allowed fields -- [ ] No new tables for warnings; warnings persist on InventorySyncRun.error_context.warnings[] (T009) +- [x] No new tables for warnings; warnings persist on InventorySyncRun.error_context.warnings[] ## Functional Requirements Coverage @@ -26,12 +26,12 @@ ## Functional Requirements Coverage ## Non-Functional Requirements Coverage - [x] NFR-001 Idempotency: re-running extraction does not create duplicates; updates metadata deterministically -- [ ] NFR-002 Unknown reference shapes handled gracefully: warning recorded in run metadata; does not fail sync; no edge created for unsupported types (T008, T009, T036) +- [x] NFR-002 Unknown reference shapes handled gracefully: warning recorded in run metadata; does not fail sync; no edge created for unsupported types ## Tests (Pest) - [x] Extraction determinism + unique key (re-run equality) -- [ ] Missing edges show “Missing” badge and safe tooltip (T019, T021) +- [x] Missing edges show “Missing” badge and safe tooltip - [x] 50-edge limit enforced and truncation behavior is observable (if specified) -- [ ] Tenant isolation for queries and UI (T012) +- [x] Tenant isolation for queries and UI - [x] UI smoke: relationship-type filter limits visible edges diff --git a/tests/Feature/DependencyExtractionFeatureTest.php b/tests/Feature/DependencyExtractionFeatureTest.php index 01d3162..e45d570 100644 --- a/tests/Feature/DependencyExtractionFeatureTest.php +++ b/tests/Feature/DependencyExtractionFeatureTest.php @@ -127,3 +127,62 @@ public function request(string $method, string $path, array $options = []): Grap $count = InventoryLink::query()->where('tenant_id', $tenant->getKey())->count(); expect($count)->toBe(50); }); + +it('persists unsupported reference warnings on the sync run record', function () { + $tenant = Tenant::factory()->create(); + + $this->app->bind(GraphClientInterface::class, function () { + return new class implements GraphClientInterface + { + public function listPolicies(string $policyType, array $options = []): GraphResponse + { + return new GraphResponse(true, [[ + 'id' => 'pol-warn-1', + 'displayName' => 'Unsupported Assignment Target', + 'assignments' => [ + ['target' => ['filterId' => 'filter-only-no-group']], + ], + ]]); + } + + public function getPolicy(string $policyType, string $policyId, array $options = []): GraphResponse + { + return new GraphResponse(true); + } + + public function getOrganization(array $options = []): GraphResponse + { + return new GraphResponse(true); + } + + public function applyPolicy(string $policyType, string $policyId, array $payload, array $options = []): GraphResponse + { + return new GraphResponse(true); + } + + public function getServicePrincipalPermissions(array $options = []): GraphResponse + { + return new GraphResponse(true); + } + + public function request(string $method, string $path, array $options = []): GraphResponse + { + return new GraphResponse(true); + } + }; + }); + + $svc = app(InventorySyncService::class); + $run = $svc->syncNow($tenant, [ + 'policy_types' => ['deviceConfiguration'], + 'categories' => [], + 'include_foundations' => false, + 'include_dependencies' => true, + ]); + + $warnings = $run->error_context['warnings'] ?? null; + expect($warnings)->toBeArray()->toHaveCount(1); + expect($warnings[0]['type'] ?? null)->toBe('unsupported_reference'); + + expect(InventoryLink::query()->where('tenant_id', $tenant->getKey())->count())->toBe(0); +}); diff --git a/tests/Feature/InventoryItemDependenciesTest.php b/tests/Feature/InventoryItemDependenciesTest.php index 8af983e..5c8ba75 100644 --- a/tests/Feature/InventoryItemDependenciesTest.php +++ b/tests/Feature/InventoryItemDependenciesTest.php @@ -3,6 +3,7 @@ use App\Filament\Resources\InventoryItemResource; use App\Models\InventoryItem; use App\Models\InventoryLink; +use App\Models\Tenant; use Illuminate\Support\Str; it('shows zero-state when no dependencies and shows missing badge when applicable', function () { @@ -27,10 +28,16 @@ 'target_type' => 'missing', 'target_id' => null, 'relationship_type' => 'assigned_to', - 'metadata' => ['last_known_name' => null], + 'metadata' => [ + 'last_known_name' => 'Ghost Target', + 'raw_ref' => ['example' => 'ref'], + ], ]); - $this->get($url)->assertOk()->assertSee('Missing'); + $this->get($url) + ->assertOk() + ->assertSee('Missing') + ->assertSee('Last known: Ghost Target'); }); it('direction filter limits to outbound or inbound', function () { @@ -109,3 +116,32 @@ ->assertSee('Scoped Target') ->assertDontSee('Assigned Target'); }); + +it('does not show edges from other tenants (tenant isolation)', function () { + [$user, $tenant] = createUserWithTenant(); + $this->actingAs($user); + + /** @var InventoryItem $item */ + $item = InventoryItem::factory()->create([ + 'tenant_id' => $tenant->getKey(), + 'external_id' => (string) Str::uuid(), + ]); + + $otherTenant = Tenant::factory()->create(); + + // Same source_id, but different tenant_id: must not be rendered. + InventoryLink::factory()->create([ + 'tenant_id' => $otherTenant->getKey(), + 'source_type' => 'inventory_item', + 'source_id' => $item->external_id, + 'target_type' => 'missing', + 'target_id' => null, + 'relationship_type' => 'assigned_to', + 'metadata' => ['last_known_name' => 'Other Tenant Edge'], + ]); + + $url = InventoryItemResource::getUrl('view', ['record' => $item], tenant: $tenant); + $this->get($url) + ->assertOk() + ->assertDontSee('Other Tenant Edge'); +}); diff --git a/tests/Unit/DependencyExtractionServiceTest.php b/tests/Unit/DependencyExtractionServiceTest.php index 860fa23..f0f83de 100644 --- a/tests/Unit/DependencyExtractionServiceTest.php +++ b/tests/Unit/DependencyExtractionServiceTest.php @@ -5,6 +5,7 @@ use App\Models\Tenant; use App\Services\Inventory\DependencyExtractionService; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Facades\Log; use Illuminate\Support\Str; uses(RefreshDatabase::class); @@ -29,8 +30,11 @@ $svc = app(DependencyExtractionService::class); - $svc->extractForPolicyData($item, $policyData); - $svc->extractForPolicyData($item, $policyData); // re-run, should be idempotent + $warnings1 = $svc->extractForPolicyData($item, $policyData); + $warnings2 = $svc->extractForPolicyData($item, $policyData); // re-run, should be idempotent + + expect($warnings1)->toBeArray()->toBeEmpty(); + expect($warnings2)->toBeArray()->toBeEmpty(); $edges = InventoryLink::query()->where('tenant_id', $tenant->getKey())->get(); expect($edges)->toHaveCount(4); @@ -43,7 +47,7 @@ expect($tuples->count())->toBe(4); }); -it('handles unsupported references by creating missing edges', function () { +it('handles unsupported references by recording warnings (no edges)', function () { $tenant = Tenant::factory()->create(); /** @var InventoryItem $item */ @@ -59,11 +63,20 @@ ], ]; - $svc = app(DependencyExtractionService::class); - $svc->extractForPolicyData($item, $policyData); + Log::spy(); - $edge = InventoryLink::query()->first(); - expect($edge)->not->toBeNull(); - expect($edge->target_type)->toBe('missing'); - expect($edge->target_id)->toBeNull(); + $svc = app(DependencyExtractionService::class); + + $warnings = $svc->extractForPolicyData($item, $policyData); + expect($warnings)->toBeArray()->toHaveCount(1); + expect($warnings[0]['type'] ?? null)->toBe('unsupported_reference'); + expect($warnings[0]['policy_id'] ?? null)->toBe($item->external_id); + + expect(InventoryLink::query()->count())->toBe(0); + + Log::shouldHaveReceived('info') + ->withArgs(fn (string $message, array $context) => $message === 'Unsupported reference shape encountered' + && ($context['type'] ?? null) === 'unsupported_reference' + && ($context['policy_id'] ?? null) === $item->external_id) + ->once(); });